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

Resolving APIs URL to different namespaces #3816

Closed
wants to merge 3 commits into from
Closed

Conversation

debnet
Copy link

@debnet debnet commented Jan 8, 2016

An enough tested revision in order to allow resolving APIs URL using APIs in different namespaces.

Used as a patch in production environment since about one year without issue.

See #1495

@xordoquy xordoquy added this to the 3.3.3 Release milestone Jan 15, 2016
@xordoquy
Copy link
Collaborator

xordoquy commented Jan 15, 2016

Open question, would it be worth to alter the tests so they include one model in common and make sure it's resolved correctly for each namespace ?

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 15, 2016

I wonder if some of the code can't be factorized with Django's.

continue
# Raise exception if everything else fails
if not url:
raise
Copy link
Collaborator

@xordoquy xordoquy Jan 15, 2016

Choose a reason for hiding this comment

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

I'd really like to see an explicit raise here.
This syntax will work as long as no other exception is being thrown - no matter whether they are caught or not.
Given the code quantity between the initial except and this line it's subject to caution imho.

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 15, 2016

OK, with the restriction on the raise statement it looks good to me.

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 15, 2016

Use case that may lead to raising the wrong exception:

Guess what raise will raise when url is None ?

Is that use case possible ?

NB:

>>> try:
...     0/0
... except Exception:
...     try:
...         'a' + 1
...     except TypeError:
...         pass
...     raise
... 
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
TypeError: cannot concatenate 'str' and 'int' objects
>>> 

@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@tomchristie
Copy link
Member

tomchristie commented Feb 15, 2016

My super quick review --> I can see a lot of code, and not much explanation. What case exactly is being resolved here? If there's any other kind of workaround or the case isn't 100% clear cut then I'm unlikely to be in favor of this change.

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 2, 2016

The point I don't get here is why it does try every namespace if it can't resolve without namespace or with the current namespace.

@debnet
Copy link
Author

debnet commented Mar 2, 2016

For example, in a case when a model from an application under a namespace references another application model under a different namespace.

@nwp90
Copy link

nwp90 commented Jan 4, 2017

I don't see how #4219 justifies closing this. This has nothing to do with versioning. If a namespace is in use, it would be nice for HyperlinkedModelSerializers to continue to function, at least where the linked detail route is in the same namespace as the current request.

I'm not sure what the right thing to do is when it's in a different namespace - it's conceivable that the current app would know about a subtree of the overall namespace but not the whole thing.

e.g. app with routes under "foo:api:firstapp" knows about the "api:firstapp" portion of the namespace, and that the model it's after is in "api:otherapp", but not that that is also now under the top-level "foo" namespace.

Seems to me that it would be desirable to infer a prefix based on what is expected and experienced (expect to be in "api:firstapp" but discover that we're in "foo:api:firstapp", so infer "foo" prefix), and then use that to help find other known routes.

@debnet
Copy link
Author

debnet commented Jan 4, 2017

In my use-case, the current version doesn't solve the problem.

I still have to deal with models referencing each others through differents namespaces myself.

Fortunatly, @xordoquy gives me the evidence that my previous solution have some caveats and can lead to undesired behaviour.

Since, I changed the whole implementation of HyperlinkField in which I have to scan all the application URLs in order to find the '-detail' API of my related models.

class CustomHyperlinkedField:
    urls_for_model = {}
    pk_field = None

    def get_name(self, obj):
        return str(obj.pk)

    def get_url(self, obj, view_name, request, format):
        if hasattr(obj, 'pk') and obj.pk in (None, ''):
            return None

        model = getattr(getattr(self, 'queryset', None), 'model', None) or type(obj)
        urls = self.urls_for_model[model] = self.urls_for_model.get(model) or list(recursive_get_urls(model=model))
        for urlname, url in urls:
            if urlname.endswith(view_name):
                view_name = urlname

        lookup_value = getattr(obj, self.lookup_field)
        kwargs = {self.lookup_url_kwarg: lookup_value}
        return self.reverse(view_name, kwargs=kwargs, request=request, format=format)

@nwp90
Copy link

nwp90 commented Jan 4, 2017

For the moment I'm working around this with:

class SBCoreRelatedField(serializers.HyperlinkedRelatedField):
    def __init__(self, view_prefix=None, view_name=None, **kwargs):
        if view_name is not None and view_prefix is not None:
            view_name = "%s:%s" % (view_prefix, view_name)
        kwargs['view_name'] = view_name
        super().__init__(**kwargs)


class SBCoreIdentityField(serializers.HyperlinkedIdentityField):
    def __init__(self, view_prefix=None, view_name=None, **kwargs):
        if view_name is not None and view_prefix is not None:
            view_name = "%s:%s" % (view_prefix, view_name)
        kwargs['view_name'] = view_name
        super().__init__(**kwargs)


class SBCoreSerializer(serializers.HyperlinkedModelSerializer):
    serializer_related_field = SBCoreRelatedField
    serializer_url_field = SBCoreIdentityField

    def __init__(self, *args, **kwargs):
        context = kwargs.get('context', None)
        super().__init__(*args, **kwargs)
        if not hasattr(self, 'serializer_namespace'):
            if context is not None:
                request = context.get('request', None)
                if request is not None:
                    self.serializer_namespace = request.resolver_match.namespace

    def build_url_field(self, *args, **kwargs):
        field_class, field_kwargs = super().build_url_field(*args, **kwargs)
        field_kwargs['view_prefix'] = self.serializer_namespace
        return field_class, field_kwargs

    def build_relational_field(self, *args, **kwargs):
        field_class, field_kwargs = super().build_relational_field(*args, **kwargs)
        field_kwargs['view_prefix'] = self.serializer_namespace
        return field_class, field_kwargs

i.e. the serializer now knows which namespace it's in, and can pass this through to the fields. If a field is referring to an object in a different namespace, I have to specify that on the field.

@nwp90
Copy link

nwp90 commented Jan 4, 2017

Oh, I've added get_extra_kwargs as below, to allow setting those prefixes from the serializer's Meta class too:

def get_extra_kwargs(self):
    extra_kwargs = super().get_extra_kwargs()
    namespaces = getattr(self.Meta, 'namespaces', None)
    if namespaces is not None:
        if not isinstance(namespaces, (dict,)):
            raise TypeError(
                'The `namespaces` option must be a dict. '
                'Got %s.' % type(namespaces).__name__
            )
        for field_name, view_prefix in namespaces.items():
            kwargs = extra_kwargs.get(field_name, {})
            kwargs['view_prefix'] = view_prefix
            extra_kwargs[field_name] = kwargs

    return extra_kwargs

@tomchristie
Copy link
Member

tomchristie commented Jan 5, 2017

Can't we enforce explicitly namespaced view names in this case rather than magically inferring them?

@debnet
Copy link
Author

debnet commented Jan 5, 2017

It depends how you want use your viewsets. In my case, we have a framework above our project in order to dynamically generate APIs from models with all filters, orders, related models and prefetchs... so having to deal with namespaces could lead to a lot of redundant work.

Nevertheless, I understand your point of view: explicit is better than implicit. But I guess through this issue I'm not alone to require that kind of feature. :)

@nwp90
Copy link

nwp90 commented Jan 8, 2017

You can't be explicit when you don't know the full namespace a pluggable app might be included under.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants