-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Add compound field types #1285
Add compound field types #1285
Conversation
Not ready to merge yet. If direction is generally accepted, will add tests. |
] | ||
|
||
|
||
class ListOrObjectField(WritableField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we drop this - ListField and DictField should be sufficient to have in core - if anyone wants something more specific that needs to be a third party library or implemented inside their project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really get the purpose of this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll be dropping it per tom's recommendation, but... there exist JSON specs that have fields that are allowed to be some type of thing, or a list of that same type. The practical example is the HAL spec where, for a resource, each link relation in the _links property may have one link or a list of many. In either case the structure of a link should follow one definition. That scenario is your basic practical example of a type-choice and this field implements it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomchristie We seriously need a cookbook. This field is a good candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thedrow - That's great, but I don't want to be the maintainer for that - someone else is more than welcome to take that on and we can link to it heavily from the main docs. I'm happy to help out with managing the domain for it, and porting the current doc buildings to a new project, so if someone starts writing up a bunch of different markdown sources for pages of a cookbook then I can help get that building into a github pages hosted docs site with a similar style to the main docs.
Good stuff, tests & docs needed ofc. |
Cool. Based on general agreement, I'll make the changes based on comments, look into the travis failure, and add tests/docs/etc. |
Oh, that was simple. Python2.6 compat :) Will fix that too. |
|
||
def __init__(self, value_field, *args, **kwargs): | ||
super(DictField, self).__init__(*args, **kwargs) | ||
self.value_field = value_field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I pondered allowing a key_field to be specified as well, but this seems of limited utility. Agree? Disagree? If people want it I can add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ignore that probably yeah. Actually I think it'd probably be worth coercing the key to a sting. In JSON that'll always be the case anyways, and if someone wants some other behavior they'll just need to implement a slightly different dict field themselves.
Made all changes based on comments so far. Because of the need to import BaseSerializer, had to move these types to a new file. I'll wait another day for more comments and then move onto doc & tests. |
Took another look at this and realized you were right about changing the signature on Worth taking that approach instead of checking the type of the field against |
Hehe. I'll switch it back soon then. I agree that changing the API of BaseSerializer.from_native is no small matter in any case, so maybe it's important to ensure the docstring is clear on the extra files argument. I could keep the new fields in the new module regardless. I definitely like it over growing the fields module endlessly. |
self.item_field = item_field | ||
|
||
def to_native(self, obj): | ||
if obj: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably check to make sure that self.value_field
has been set. This also applies to the other to_native
and from_native
methods. Otherwise the default of None
will throw errors any time data is passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the element expression in the list comprehension below does a check on the field. if it's none, the list items are just passed through. Now, it would probably cleaner to just check it up front... I'll probably just do that.
* Remove ListOrObjectField * Allow item_field & value_field to be optional * Undo from_native API change; flex invocation in compound fields * In DictField, coerce key to a string * If list or dict is None or empty, just return input preserving empty containers * Remove dict comprehension for Python 2.6 compat
* In ListField and DictField methods, place field check before list or dict construction
…y processing to a unicode string
Updates:
You can start reviewing the doc (it's not a lot) at your leisure while I work on tests (also shouldn't take too long). |
fields. | ||
""" | ||
from .fields import WritableField | ||
from .serializers import BaseSerializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this import can be dropped at this point, I don't see it being used anywhere else in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right-o, fixed.
Actually, back to implementation a bit. I need to at least account for validation (e.g., to ensure item-field validation is applied to list items). Also, looking at the other types attributes, and wondering how much of a gap it is that I don't specify:
Thoughts on that? Okay to assume consumers need to map these types to HTML forms themselves or is there reasonable help I should provide? What about those other attribute? |
I was assuming these composite types wouldn't (be able to) support form inputs (in the same way that nested serializers don't support form inputs) since they don't map nicely to HTML form field inputs. That's not really an issue - we can document that limitation, and perhaps also ensure that just the raw data form (typically JSON input) is displayed in the browsable API. |
With doc, tests, and validation done I think it's ready for some more review. |
It does seem odd, going forward, to have both ListType and the "many" argument to serializers. |
Actually, that's an interesting thought... should MySerializer(many=True) be expected to work identical to ListField(MySerializer()) ? Should one option or the other be dropped for 3.0? |
The build fails on anything other than Python 2.7: https://travis-ci.org/tomchristie/django-rest-framework/builds/16090819 |
@kevin-brown Yup, fixed, I think. I'll track the builds and watch for any more problems. |
Looking good. One thing that isn't clear to me from the documentation is if the validation fails for one or more of the elements, how I should expect the Looking at the scope of this I'm also coming down more in favour of a third-party package. (I realise this is a change of heart, as I considered that this could be in scope enough to be included in core) It seems like the sort of package where there may be various edge cases etc which I'd rather not have to end up thinking about myself. Wondering how you'd feel about that @estebistec ? Would it be something you'd be up for maintaining as a seperate package, that we then link to from the |
You're right. I should make this more explicit in the doc. Will do.
I agree that some interesting edge cases can come up, and I thought of some of them as I wrote the tests and code for this. The biggest mismatch to me is, now that we have ListField, when would you use ListField with a serializer class vs. just giving the many=True argument. Yet, there was never a way to just create a list of a simpler (non-serializer) type, so this is still needed. What other edge cases occurred to you? (or were you just generally getting the feeling of a minefield :P). I feel like we should be able to ask the questions that determine what is making this feel non-core, or safer as 3rd-party for now. All of that said, I would be quite okay with making this a 3rd-party package. The code is isolated enough so that none of what I've done is really throw-away. However, I could still use the change to BaseSerializer.from_native to make its API change to that method more passive. When we stumbled across that you seemed unhappy with the realization of the API mismatch as well. |
Mostly just that there's a fair amount of code, docs & tests, which all adds up to extra maintenance.
Yes I think that change would need to stay in. |
In that case I'll start on the 3rd-party package soon. Want me to just use this issue for the tiny change that remains, or close this one and open a new pull? New, focused one is probably better... |
Agreed. |
Pull #1329 created for the simple change. Closing this guy. Will report back on the users list when I've got the 3rd party package ready to be community-reviewed, linked in the doc (that'll be another pull), etc. |
I'll be interested to take a look at this when you have it ready. |
@estebistec +1 would love to test out this package |
@paulmelnikow @adamJLev Yes, I'm still planning on this. The thing that was closed down here is being used smoothly at my work. What I've been pondering as a pre-cursor is: how do I reconcile that fact that ListField will overlap with many=True for serializers? Of course DictField is entirely new. @tomchristie, how do you feel about my style of implementation here? Would you prefer this simpler approach, or should one try to create many=True support for fields, similar to what serializers have? If the simpler approach were to be successful, it it possible any of this could come back into DRF? What of the possible confusion of choosing between ListField and many=True? Does the complexity of many=True include functionality that I'm missing? |
I'd generally much prefer to see new functionality being maintained in third party repos. Sure it could come back into REST framework core, but I don't see any reason ATM why it wouldn't be just as good in a third party repo. I would rather ensure that whatever documentation we have linking to it is sufficiently obvious and includes enough content that folks will be happy to just go ahead and pip install the extra fields if needed.
I'm not really worried about that. The keyword style many=True is a syntax that makes particular sense in the domain of model relationships (rather than generically for any field types). If ListField(...) makes more sense in some other contexts that's just fine. |
Okay, that makes sense. Then I think the direction I'll go is constraining ListField to other non-serializer fields. DictField can be anything. Thanks @tomchristie |
Very welcome. Looking forward to seeing the package - it's a bit of a gap ATM. |
We're currently pulling the We ran across a small bug with the def from_native():
...
if self.item_field and data:
return [self.item_field.from_native(item_data) for item_data in data] This was causing a |
Looking forward to this package, for now also using the I'm curious though, how are you thinking to handle list http form params?
Image a post request with fruits params like this: The key problem here is that when you say |
Okay, sorry for the delay on this. Working on it this weekend :P |
@adamJLev why would we want this serializer logic to take on logic that belongs nearer to the renderers? But, I am building out the new project today, and this issue is closed, so hopefully I'll have a more proper place to discuss such things soon... |
Okay folks! Let's move this party over here: https://github.com/estebistec/drf-compound-fields. @mwarkentin: do you mind logging an issue there for the bug you found? @tomchristie I've logged this [1] issue to update docs here when that project is released. So once I get far enough expect to see a pull here for that. I think that covers everything here. Thanks all for the comments! |
@mwarkentin thanks! |
Implementation for #560