Added the ability to specify a resource_name on Models

[scottfisk]

I mentioned this yesterday on issue #74 and figured I would just open a PR to get opinions on this change.

This would allow a resource_name to be specified on the model. This was required in certain cases (relations: this line) where no serializer or view could have overridden the resource_name. This would allow the resource_name of a model to be overridden globally for that model.

I haven't written any tests yet as I uncovered a lot of places the model __name__ property is used, hence WIP.

EDIT:
Test TODOs:

• Model resource_name used on a relation
• Model resource_name in included document
• Serializer resource_name override over Model resource_name
• View resource_name override over Model resource_name

[scottfisk]

Turns out that it is fairly hacky to add a custom property to the Meta class of a Django Model. So this approach is not as clean as I thought.

Alternatives I can see right now is hooking into the existing verbose_name on the Meta class of a django model as the default instead of using __name__ as is done now. (This is actually how the other python drf-jsonapi adapter determines resource name of a model) The inflection translation will have to be tweaked a bit since the verbose name returns names with spaces: model type instead of ModelType.

[jsenecal]

@scottfisk I'm wondering if this realy is a good way to deal with the resource name.
using verbose_name seems a bit too verbose for me...

[scottfisk]

@jsenecal Yeah I don't really like the state this got to either. Unfortunately you can't add something like resource_name on model.Meta cleanly in Django.
Perhaps for now it is best to just make the call that we cannot rename resources at the model level. I think the implications of that is that there are quite a few places, like relations, where the model name will always be used as the resource name

[leifurhauks]

@scottfisk, since adding custom properties to Meta is impractical, what
about either 1) putting it in a class attribute on the model class itself
or 2) putting it in a different nested class on the model class? I've seen
django packages use both techniques as an alternative to hacking the Meta
class.

An example of 2) would be

class MyModel(models.Model):
    class Meta:
        # my meta options

    class JSONAPIMeta:
        resource_name = 'custom_resource_name'

On 27 November 2015 at 12:08, Jonathan Senecal [email protected]
wrote:

@scottfisk https://github.com/scottfisk I'm wondering if this realy is
a good way to deal with the resource name.
using verbose_name seems a bit too /verbose/ for me...

—
Reply to this email directly or view it on GitHub
#152 (comment)
.

[jsenecal]

I personally like option 2 a lot 👍

[scottfisk]

@leifurhauks Im open to updating this PR to approach 2 if the maintainers think that would be preferable.

EDIT: Just saw @jsenecal's message. I'll modify this PR to do that. 👍

[jsenecal]

Thanks to both of you @scottfisk @leifurhauks

[e3b0c442]

A thought -- I see in the current PR all references to _meta.model.__name__ have been replaced by calls to the new mechanism (I know it hasn't been updated for recent discussion).

It may be appropriate to keep the original _meta.model.name as a fallback in case the JSONAPIMeta doesn't exist. Thoughts?

[scottfisk]

@bierdybard Yeah that makes sense to me. I plan to do that when I update this PR (early this week). Keeps the fallback path basically the same as on master.

[mkohram]

How about the model JSONAPIMeta taking precedence over serializer and view resource_name. I think I have a test that justifies this.

[jerel]

Making JSONAPIMeta take precedence over the serializer's resource_name would prevent a serializer from setting its own resource_name, right? I use different serializers for different API authorization levels and I could imagine setting a resource_name of identities for superadmins and users for normal users.

[jsenecal]

@jerel One could use proxy models and then have different serializers to point to them...

[jerel]

@jsenecal right, proxy models would work fine if we go that route. I tend to think of models as the core with serializers next and views at the outer layer. So the natural priority seems like view > serializer > model

[jsenecal]

So the lookup order shall be what exactly ?
1- Look for JSONAPIMeta in model
2- Look for resource_name in serializer
3- Look for the view name

I think it is important we discuss that in detail here.

[mkohram]

Here is where the conflict occurs: A ResourceRelatedField gets its type from the model (no access to the model serializer) but when it gets included the type would come from the serializer. So now you have the same resource in one payload with two different types. right? I could give an example if that just sounded like nonsense!

[mkohram]

BTW, this is the case where JSONAPIMeta is not defined and resource_name is defined on the serializer.

[jsenecal]

So, maybe we should drop ressource_name on serializers altother, for consistency ?

[mkohram]

what about a serializer that is not tied to a model?

[jsenecal]

@mkohram How would you use ResourceRelatedField then ?

[jerel]

@jsenecal I was thinking the reverse of that:

1. Look at view
2. Look at serializer
3. Look at model

Typically it would get derived from the model unless there was a need to set it at the serializer or view level.

@mkohram that makes sense. I hadn't thought of that. I guess we could either add resource_name as a kwarg on ResourceRelatedField or always do the config on the model for simplicity

[jsenecal]

@jerel That would work for me

[mkohram]

@jerel Just to be clear we are still talking about keeping JSONAPIMeta right?

[jsenecal]

@mkohram yes, I am

[jerel]

@mkohram yes, I was also

[mkohram]

This makes sense to me too. Defining resource_name on serializers will generally cause other problems so if there was a way to not allow it for ModelSerializer, I know that's out there though!

[scottfisk]

So I switched this over to using JSONAPIMeta.resource_name (easy change).

I don't know what all you want me to include in this PR but I think:

• The precedence of resource_name still needs to be figured out. Sounds like we are going to rip them out of serializers in favor of proxy models?
• More tests for all this rename model stuff
• Some tox versions are failing with a cannot import name OrderedDict error. I don't think that was my changes and I recall something getting PRed to master a while back for that. I ll see if its as simple as a rebase or merge from develop

EDIT: Can't rebase off develop since I am targeting a branch.

[mkohram]

I think I have a good start.

8 tests up to now:

Included document and relation type match for all combinations (4)
Document type and relation type match for all combinations (4)

The other cases should be simpler to write.

[scottfisk]

Alright, pretty big update on the tests (thanks @mkohram) and a couple fixes.

I would like some feedback on my solution to the serializer overriding model when used with include issue. At the moment I did fe0056d. I think this covers the weird edge case by always using a serializer's resource_type even if it is not being requested as part of the ?include=.

[mkohram]

Thanks a lot for the effort. This is looking good to me. @jerel, @jsenecal need to comment. BTW, do you know why the build is failing? Also the consistency tests don't test the view resource_name. Do we need tests for that as well?

[jerel]

@scottfisk @mkohram thanks to both of you for your significant effort on this. It looks like the only trouble with the build is due to Django 1.7. I don't see the error right away but it looks like it's related to pagination. If you do pip install django==1.7.11 and then py.test -vv on your local it should be easy to spot the diff and hopefully track it down

[scottfisk]

@jerel @mkohram Ughh, fixing that test in django 1.7 was incredibly annoying to track down. I ended up just refactoring the tests we added to use pytest-django's client fixture which seemed to work. It seemed something was not cleaning up properly with the DRF test client in 1.7.

Also, is there any testing we should do on POST/PUTs. Is the resource_name ever used to infer the type of model being created? If so that may not be working properly.

[jerel]

The parser uses utils.get_resource_name so it should work the same on input as it does on output.

Thanks a lot for tracking that failure down and for the rest of the work here. @jsenecal any last words? If there's no objections in the meantime I'll merge after I get back from Christmas break

[jsenecal]

Great work on this everyone and Merry Christmas :) @jerel this looks great to me so I'll merge right away!

[mkohram]

Thanks everyone especially @scottfisk. This wasn't straightforward. Still missing a couple of tests I believe - combinations of resource_name on the view/serializer/model, we only have serializer/model combinations now - I'll add them at some point. We also need some documentation for JSONAPIMeta but that's not news 😃. Happy Holidays!

[mkohram]

@jsenecal Was this ever merged into master?

[jerel]

@mkohram it looks like the original PR targeted the feature/compound_documents branch and we accidentally merged to that without subsequently merging to develop. I opened a replacement PR but it has two small merge conflicts that we'll need to resolve before merging

[jsenecal]

@mkohram @jerel - Woops, indeed - Thanks for #197