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

Add @querystring endpoint that dumps p.a.querystring config #759

Merged
merged 2 commits into from Aug 30, 2019

Conversation

@lukasgraf
Copy link
Member

commented Jun 21, 2019

The @querystring endpoint returns the querystring config of plone.app.querystring.

Instead of simply exposing the querystring related field and operation entries from the registry, it serializes them the same way the @@querybuilderjsonconfig from p.a.querystring does.

This form is structured in a more conventient way for frontends to process:

  • Vocabularies will be resolved, and their values will be inlined in the values property
  • Operations will be inlined as well. The operations property will contain the list of operations (dotted names), and the operators property will contain the full definition of each of those operations supported by that field.
  • Indexes that are flagged as sortable are listed in a dedicated top-level property sortable_indexes

See Docs build of the documentation / example response

Closes #754

@lukasgraf lukasgraf requested a review from robgietema Jun 21, 2019
@mister-roboto

This comment has been minimized.

Copy link

commented Jun 21, 2019

@lukasgraf thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

Copy link
Member

left a comment

querystring.req and querystring.resp should be removed since we have the _get versions which are correct, for the rest it looks good!

@lukasgraf lukasgraf force-pushed the add-querystring branch 2 times, most recently from 6708108 to 11df6ca Jun 21, 2019
@lukasgraf

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Thanks! Updated & force pushed 👍

@lukasgraf

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@jenkins-plone-org please run jobs

@lukasgraf lukasgraf force-pushed the add-querystring branch from 11df6ca to 4de369d Jun 22, 2019
@lukasgraf lukasgraf force-pushed the add-querystring branch from 4de369d to 2418f5a Jun 22, 2019
@lukasgraf

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

@jenkins-plone-org please run jobs

@coveralls

This comment has been minimized.

Copy link

commented Jun 22, 2019

Coverage Status

Coverage decreased (-0.5%) to 95.75% when pulling 2418f5a on add-querystring into 1f6f72b on master.

@coveralls

This comment has been minimized.

Copy link

commented Jun 22, 2019

Coverage Status

Coverage increased (+0.008%) to 96.258% when pulling 2418f5a on add-querystring into 1f6f72b on master.

@bloodbare

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

@tisto We not going to support querystring at guillotina project, we already discussed to have a search endpoint that supports all kind of indexes and operations.

https://docs.google.com/document/d/1xooubzJKBnUzVlsa2f0GSwvuE1oir9hUdWUf1SU9S6E/edit?usp=sharing

@bloodbare

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

And we have a collection that that supports the search format to store the query.

@sneridagh

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Just tested it with the corresponding Volto branch and it works well.

Only one question: For the vocabularies we used "the old way", like just include them in the response. This is different on how we implemented it this spring, so we provide a source URL for retrieving the values. I wonder if we might find the same culprits we found in the vocabularies widgets. e.g. Users vocabulary, and if you use a custom criteria with a custom vocabulary that returns an insane amount of results.

This might complicate the widget, but it's something that we might think of upfront.

/cc @tisto @lukasgraf @robgietema @buchi

@sneridagh sneridagh referenced this pull request Aug 27, 2019
@tisto

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@plone/restapi-team opinions on @sneridagh's post above? Otherwise I'd merge and release this now since it is needed for the new collection type in Volto 4.

@tisto tisto merged commit e711b74 into master Aug 30, 2019
8 checks passed
8 checks passed
Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
Plone Jenkins CI - pull-request-5.2 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.2-3.6 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.2-3.7 Job finished with success status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 96.258%
Details
@tisto tisto deleted the add-querystring branch Aug 30, 2019
@lukasgraf

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@sneridagh sorry, I was on holidays and didn't read my mails for a while.
I don't quite understand your question though: Vocabulary contents aren't (shouldn't) be inlined in the response of the @querystring endpoint - only the vocabulary name is included in the response. Or did I misunderstand what you mean?

@sneridagh

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@lukasgraf oh ok since in your first comment in this thread you said: Vocabularies will be resolved, and their values will be inlined in the values property I thought they were inlined and I didn't check them out. So that's settled, sorry for the confusion!

@lukasgraf

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

@sneridagh oh, now I see what you mean, you're right. I'm the confused one :)

That is indeed a bit inconsistent, and could potentially be undesirable for large custom vocabs.

We already have one other inconsistency like that though: Sources (instead of vocabularies) currently get inlined in the @types endpoint response. They have to be, because there's no @sources endpoint yet :)

So, I'm currently working on that anyway (new @sources and @querysources endpoints). I would recommend that we first add these new endpoints and link to them (non-breaking change), and at a later point stop inlining vocab values everywhere, at least by default (breaking change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.