Apply query_params to plural field. Fixes #219

[flyte]

Applies the query_params to the plural field as well.

A more ideal solution might be to separate out the singular and plural query helpers, as some query params might not be appropriate for both queries.

The main 'bug fix' for this is to use the kwargs in resolve_queryset() to enable custom filtering. In order to make this work, I needed to remove in_site from kwargs by setting them as a keyword argument on resolve_pages(), where that flag is actually used. Even if you don't want to alter the API behaviour of register_query_field(), this functionality makes it possible to create the separate helpers, as I mentioned above.

[flyte]

I just noticed that there's already a singular helper, and that I hadn't made the same change to the paginated helper. Please don't merge yet, as I'll make this missing change and add a 'plural only' helper too.

[flyte]

OK, I've reverted register_query_field() to its original behaviour and added register_plural_query_field().

There's quite a lot of duplicated code in these helpers, which isn't necessarily bad, but they do have some subtle differences in behaviour that I noticed:

• Default query parameters are different between each helper
• Differences between returning a result or None on a singular query if multiple results are returned
• Different handling of urlPath queries
• No default 'order' on non-Page model based paginated queries on register_paginated_query_field()'s resolve_plural() which raises a warning if using a QuerySet that's not ordered by default, like on snippets: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list

Hopefully this hasn't changed the Python API too drastically, but it's made Grapple a lot more flexible for my use-case.

[zerolab]

Hey @flyte, thank you for this.
Will aim to review in the coming week or so

[zerolab]

Hey @flyte, thank you for this. Lots of good changes 🚀

While I appreciate them, I think the direction needs to change a bit as per my other comment. tl;dr make register_query_field and register_paginated_query_field work, rather than an all new decorator

[zerolab]

My main gripe with this approach is that it introduces yet another decorator, when it should not need to.

@register_query_field supports plurality via the plural_field_name kwarg, so we should make that work.

register_singular_query_field is a convenience decorator for very narrow uses cases, such as Wagtail Page models with max_count = 1

Finally, #219 (comment) is an important use case, which this particular example covers, but as mentioned above it would be better to handle within the existing decorators, so register_paginated_query_field

[dopry]

@zerolab on the other hand it may be nice to separate @register_singular_query_field and @register_plural_query_field to make it simpler to document. Decorators in my experience tend to do discrete things. Having one decorator that sets up two different fields means we need to explain how both behave under one heading when each is going to have distinct arguments and return types.

Where as if we had both individual decorators, then the aggregate decorator can be explained by linking to both... just a thought... Maybe not something to include in this fix, but something to think about long term.

an example of our existing docs

class grapple.helpers.register_query_field(field_name, plural_field_name=None, query_params=None, required=False, plural_required=False, plural_item_required=False, middleware=None)

all of those plural_* fields are only valuable if plural_field_name is set and have nothing to do with a singular fields

I almost feel like we should have @register_model_query_singular, @register_model_query_plural, @register_model_query_paginated. So users can choose to some something like...

advert_query_params = {
   'blog__id': graphene.Int()
}
@register_model_query_singular("advert", required=True, query_params=advert_query_params)
@register_model_query_plural("allAdverts", required=True, query_params=advert_query_params)
@register_model_query_paginated("adverts", required=False, query_params=advert_query_params, item_required=True, )
class Advert: 
   # ...
   pass

[dopry]

@flyte do you still have any inclination to work on this issue?

[flyte]

@dopry hi, I'm afraid I don't now. The project I was working on is complete and I'm no longer using this.

[dopry]

@flyte that is cool. Life happens. Is it okay if another contributor takes over this work?

[flyte]

Yes, of course! 😊