Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/28 local base urls #29

Merged
merged 6 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,20 @@ or override the loader on a per-field basis:
loader=RequestsLoader()
)

Local and remote urls
---------------------

If several services are hosted within the same domain, it could be tricky to separate
local and remote urls. In this case an additional setting ``LOOSE_FK_LOCAL_BASE_URLS``can be used
to define an explicit list of allowed prefixes for local urls.

.. code-block:: python

LOOSE_FK_LOCAL_BASE_URLS = [
"https://api.example.nl/ozgv-t/zaken/",
"https://api.example.nl/ozgv-t/catalogi/",
]


.. |build-status| image:: https://github.com/maykinmedia/django-loose-fk/workflows/Run%20CI/badge.svg
:alt: Build status
Expand Down
7 changes: 6 additions & 1 deletion django_loose_fk/constraints.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from django.db import models
from django.db.models.constraints import BaseConstraint
from django.db.utils import DEFAULT_DB_ALIAS


class FkOrURLFieldConstraint(BaseConstraint):
Expand Down Expand Up @@ -35,7 +36,7 @@ def deconstruct(self):
)
return (path, args, kwargs)

def _get_check_constraint(self, model, schema_editor):
def _get_check_constraint(self, model, schema_editor=None):
"""
Return the underlying check constraint generated.
"""
Expand Down Expand Up @@ -66,6 +67,10 @@ def remove_sql(self, model, schema_editor):
check_constraint = self._get_check_constraint(model, schema_editor)
return check_constraint.remove_sql(model, schema_editor)

def validate(self, model, instance, exclude=None, using=DEFAULT_DB_ALIAS):
check_constraint = self._get_check_constraint(model)
return check_constraint.validate(model, instance, exclude, using)

def __repr__(self):
return "<%s: field=%r>" % (self.__class__.__name__, self.name)

Expand Down
7 changes: 4 additions & 3 deletions django_loose_fk/drf.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

Provides a custom field.
"""

import logging
from dataclasses import dataclass
from typing import Tuple, Union
Expand All @@ -20,7 +21,7 @@

from .fields import FkOrURLField, InstanceOrUrl
from .loaders import FetchError, FetchJsonError
from .utils import get_resource_for_path
from .utils import get_resource_for_path, is_local

logger = logging.getLogger(__name__)

Expand All @@ -45,8 +46,8 @@ class Resolver:

def resolve(self, host: str, url: str) -> models.Model:
parsed = urlparse(url)
is_local = parsed.netloc == host
return self.resolve_local(parsed) if is_local else self.resolve_remote(url)
local = is_local(host, url)
return self.resolve_local(parsed) if local else self.resolve_remote(url)

def resolve_local(self, parsed: ParseResult) -> models.Model:
return get_resource_for_path(parsed.path)
Expand Down
6 changes: 3 additions & 3 deletions django_loose_fk/filters.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
"""
Filter support for django-filter.
"""

import logging
from functools import reduce
from urllib.parse import urlparse

from django import forms
Expand All @@ -12,7 +12,7 @@
from django_filters.filterset import FilterSet, remote_queryset as _remote_queryset

from .fields import FkOrURLField
from .utils import get_resource_for_path, get_subclasses
from .utils import get_resource_for_path, get_subclasses, is_local

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -74,7 +74,7 @@ def get_filters(self, model_field, parsed_values, host) -> dict:

filters = {}
for value in parsed_values:
local = value.netloc == host
local = is_local(host, value.geturl())
if local:
local_object = get_resource_for_path(value.path)
if self.instance_path:
Expand Down
1 change: 1 addition & 0 deletions django_loose_fk/inspectors/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Requires drf-yasg, which can be installed as the [openapi] optional group
dependency.
"""

from django_filters.rest_framework.backends import DjangoFilterBackend
from drf_yasg import openapi

Expand Down
5 changes: 5 additions & 0 deletions django_loose_fk/loaders.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def is_local_url(self, url: str) -> bool:
"""
allowed_hosts = settings.ALLOWED_HOSTS
parsed = urlparse(url)
local_base_urls = getattr(settings, "LOOSE_FK_LOCAL_BASE_URLS", [])

if any(pattern == "*" for pattern in allowed_hosts):
warnings.warn(
Expand All @@ -54,6 +55,10 @@ def is_local_url(self, url: str) -> bool:
"break django-loose-fk's behaviour. You should use an explicit list.",
RuntimeWarning,
)
return validate_host(parsed.netloc, allowed_hosts)
annashamray marked this conversation as resolved.
Show resolved Hide resolved

if local_base_urls:
return any(url.startswith(base_url) for base_url in local_base_urls)

return validate_host(parsed.netloc, allowed_hosts)

Expand Down
8 changes: 8 additions & 0 deletions django_loose_fk/lookups.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,14 @@ def get_prep_lookup(self):
if self.rhs_is_direct_value():
# If we get here, we are dealing with single-column relations.
self.rhs = [get_normalized_value(val)[0] for val in self.rhs]

else:
alextreme marked this conversation as resolved.
Show resolved Hide resolved
# we're dealing with something that can be expressed as SQL -> it's local only!
target = self.lhs.target
db_table = target.model._meta.db_table
fk_lhs = target._fk_field.get_col(db_table)
target_field = fk_lhs.target.target_field.name
self.rhs.set_values([target_field])
return super().get_prep_lookup()

def as_sql(self, compiler, connection):
Expand Down
17 changes: 17 additions & 0 deletions django_loose_fk/utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from urllib.parse import urlparse

from django.conf import settings
from django.db import models
from django.http import HttpRequest
Expand All @@ -7,6 +9,21 @@
from rest_framework.request import Request


def is_local(host: str, url: str) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a few unittests for only this method? I'm all in favor of integration tests but this feels complicated enough to test separately as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are added

"""
Define if the url is local or external based on LOOSE_FK_LOCAL_BASE_URLS
setting or a host
"""
parsed = urlparse(url)
local_base_urls = getattr(settings, "LOOSE_FK_LOCAL_BASE_URLS", [])
# if local base urls are defined - use them
if local_base_urls:
return any(url.startswith(base_url) for base_url in local_base_urls)

# otherwise use hostname
return parsed.netloc == host


def get_viewset_for_path(path: str) -> viewsets.ViewSet:
"""
Look up which viewset matches a path.
Expand Down
43 changes: 43 additions & 0 deletions tests/test_api.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Test the API interface to handle local/remote references.
"""

from unittest.mock import patch

from django.test import override_settings
Expand Down Expand Up @@ -191,3 +192,45 @@ def test_filter_zaaktype_local_fk(api_client):

assert len(response.data) == 1
assert response.data[0]["url"] == f"http://testserver.com{zaak_url}"


@patch("django_loose_fk.utils.get_script_prefix", return_value="/subpath/")
@override_settings(
ALLOWED_HOSTS=["testserver.com"],
LOOSE_FK_LOCAL_BASE_URLS=["http://testserver.com/subpath/zaaktypes/"],
)
def test_write_local_url_with_local_base_urls(mock, api_client):
url = reverse("zaak-list")
zaaktype = ZaakType.objects.create(name="test")
zaaktype_url = reverse("zaaktype-detail", kwargs={"pk": zaaktype.pk})

data = {"name": "test", "zaaktype": f"http://testserver.com/subpath{zaaktype_url}"}

response = api_client.post(url, data, HTTP_HOST="testserver.com")

assert response.status_code == 201
zaak = Zaak.objects.get()
assert zaak.zaaktype == zaaktype


@patch("django_loose_fk.utils.get_script_prefix", return_value="/subpath/")
@override_settings(
ALLOWED_HOSTS=["testserver.com"],
LOOSE_FK_LOCAL_BASE_URLS=["http://testserver.com/subpath/zaaktypes/"],
)
def test_write_remote_url_with_local_base_urls(mock, api_client):
"""
test that service with the same host and prefix can still be external
"""
url = reverse("zaak-list")
zaaktype_url = "http://testserver.com/subpath/service/zaaktypes/10/"
data = {"name": "test", "zaaktype": zaaktype_url}

with requests_mock.Mocker() as m:
m.get(zaaktype_url, json={"url": zaaktype_url, "name": "test"})

response = api_client.post(url, data, HTTP_HOST="testserver.com")

assert response.status_code == 201
zaak = Zaak.objects.get()
assert zaak.zaaktype == zaaktype_url
1 change: 1 addition & 0 deletions tests/test_chain_loose_fk.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Test the API interface to handle chained local/remote references.
"""

import pytest
import requests_mock
from rest_framework.reverse import reverse
Expand Down
1 change: 1 addition & 0 deletions tests/test_compare.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Test the API interface to handle comparing of loose-fk urls
"""

import pytest
import requests_mock

Expand Down
56 changes: 56 additions & 0 deletions tests/test_is_local.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
"""
Test django_loose_fk.utils.is_local function with various
combinations of settings
"""

from django.test import override_settings

from django_loose_fk.utils import is_local


@override_settings(
LOOSE_FK_LOCAL_BASE_URLS=["http://api.example.nl/ozgv-t/zaken"],
)
def test_with_setting_same_host_same_prefix():
assert (
is_local(host="api.example.nl", url="http://api.example.nl/ozgv-t/zaken/1")
is True
)


@override_settings(
LOOSE_FK_LOCAL_BASE_URLS=["http://api.example.nl/ozgv-t/zaken"],
)
def test_with_setting_same_host_diff_prefix():
assert (
is_local(host="api.example.nl", url="http://api.example.nl/ozgv-t/documenten/1")
is False
)


@override_settings(
LOOSE_FK_LOCAL_BASE_URLS=["http://otherapi.example.nl/ozgv-t/zaken"],
)
def test_with_setting_diff_host():
assert (
is_local(host="api.example.nl", url="http://otherapi.example.nl/ozgv-t/zaken/1")
is True
)


def test_no_setting_same_host_with_prefix():
assert (
is_local(host="api.example.nl", url="http://api.example.nl/ozgv-t/zaken/1")
is True
)


def test_no_setting_same_host_no_prefix():
assert is_local(host="api.example.nl", url="http://api.example.nl/zaken/1") is True


def test_no_setting_diff_host():
assert (
is_local(host="api.example.nl", url="http://otherapi.example.nl/ozgv-t/zaken/1")
is False
)
1 change: 1 addition & 0 deletions tests/test_model_field_interface.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Test that it's possibly to handle remote/local objects transparently.
"""

import uuid

import pytest
Expand Down
1 change: 1 addition & 0 deletions tests/test_querying.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Test the ORM queries against the virtual field.
"""

import pytest
import requests_mock

Expand Down
Loading