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

Refactor _store, extract model creation logic from JSON logic #87

Open
stefanmajoor opened this issue Dec 7, 2017 · 2 comments
Open

Comments

@stefanmajoor
Copy link
Collaborator

At this moment the _store method is a bit weird. It expects parsed JSON as input, but formats the output as JSON. Although it is difficult to change the signature of _store, I propose adding a second function which doesn't format the code as JSON.

E.g.

def _foo(self, obj, values, request, ignore_unknown_fields=False, pk=None): 
    # returns bar object
def _store(**kwargs)
    model = _foo(**kwargs)
    data = self._get_obj(obj.pk, request=request)
    data['_meta'] = {'ignored_fields': ignored_fields}
    return data

Now the _foo method can be used in the rest of the code. This removes the restriction that the bar object always needs to return an object which can be returned. That is a hidden restriction, which may not be obvious at first, but let me demonstrate with two example:

One use case is the UserViews. At this moment we have User and Profile models. The _get_objects from a user depends on the profile model. However, I can not create a profile model before an user model is created. Thus I can create neither a profile nor a user model. At this moment there is an hack in place, where signals are used to automatically create a profile object when an user model is used. This is very magical, and doesn't work if you have non-default values on the profile model. Adding such a method, would allow the logic to move from the signals, to the parent's _store function, where it belongs.

A second use case is when the user and profile model both needs to be changed. At this moment this means that you need to call _store on both the user and profile model. Then you need to do an additional _get_obj call, to reflect the changed made in both stores. It would be neater to not have the superfluous _get_obj call in binder's store.

@stam
Copy link
Contributor

stam commented Dec 14, 2017

At this moment there is an hack in place, where signals are used to automatically create a profile object when an user model is used. This is very magical, and doesn't work if you have non-default values on the profile model. Adding such a method, would allow the logic to move from the signals, to the parent's _store function, where it belongs.

Doesn't this proposed change break createsuperuser?

@stefanmajoor
Copy link
Collaborator Author

Doesn't this proposed change break createsuperuser?
Indeed it does, which can be worked around. That issue is however not the point of this ticket. It was just one example of a reasonable use case in which this occurs. The bigger issue is that one-to-one relationships can not correctly be implemented if the _get_objects function depends on the one-to-one relation to be correctly set. That is a non-trivial, and unnecessary constraint IMO

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

No branches or pull requests

3 participants