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

Search #48

Merged
merged 17 commits into from
Apr 18, 2016
Merged

Search #48

merged 17 commits into from
Apr 18, 2016

Conversation

lukasgraf
Copy link
Member

This implements a named GET service /search that can be invoked on any context to search for Plone content.

Usage examples:

GET /plone/search?SearchableText=lorem
GET /plone/search?created.query=1949-01-01&created.query=1951-01-01&created.range=min%3Amax
GET /plone/folder/search?sort_on=path&metadata_fields=_all
  • Search is contextual by default (constrained by path of context, unless an explicit path query is specified)
  • Nested dictionaries can be specified via dotted notation (path.query=foo&path.depth=1) similar to ZPublisher's record arguments, but without the need for the :record suffix
  • Optional type information (if required because of data type ambiguity) can be supplied in ZPublisher's argument conversion style (numeric_index=42:int), but should rarely be required
  • Search results are represented as compact summaries by default, additional metadata columns that should be retrieved can be specified via the metadata_fields parameter

See the documentation update included in this PR for more details on user-facing usage.

@lukasgraf
Copy link
Member Author

@tisto @buchi @bloodbare

This one is ready for review.

FYI: After some back and forth, I decided to drop support for POST + JSON body for the following reasons:

  • "There should be one-- and preferably only one --obvious way to do it."
  • JSON only helps to retain some typing information - datetimes still need to be dealt with
  • No way to build links to search queries with POST - not what you want in a hypermedia API
  • Implementation is trivial, but we'd still need to test, document and maintain two HTTP verbs instead of one

However, if a real need arises, it would be trivial to add support for POST later.

@lukasgraf lukasgraf changed the title Search (GET and POST) [WIP] Search Apr 5, 2016
@ebrehault
Copy link
Member

@lukasgraf, using a GET with querystring parameters is what REST APIs usually do and not POST (one of the reasons is POST requests cannot be cached, as caching mechanisms are always based on the URL).

@buchi
Copy link
Member

buchi commented Apr 8, 2016

There's a hard dependency on Products.DateRecurringIndex in query.py which breaks compatibility with Plone 4.3. Can you make this optional? See also #72

Didn't have time for a in-depth review yet, will come back later...

@lukasgraf
Copy link
Member Author

@buchi will do, thanks for the hint!

Complex queries that consist of dicts with additional query
options need to be flattened when submitted via query string,
and deserialized again on the the server side.
By default (i.e. unless the user explicitly supplies a
'path' with the query) the search will be constrained
to the subtree it was invoked on.
For indexes that support query values or query options
that are of non-string types, we need to reconstruct
that type information after it got lost over HTTP.
maintain compatibility with vanilla Plone 4.3.
@lukasgraf
Copy link
Member Author

Made DateRecurringIndexQueryParser optional and rebased onto master.

@tisto
Copy link
Sponsor Member

tisto commented Apr 18, 2016

Hey @lukasgraf, sorry for messing up your pull request. The build was broken and I tried to fix it by upgrading to the latest Plone versions (4 and 5). It turned out the plone.app.contenttypes fixtures got a major refactoring by @pbauer. I pinned plone.app.contenttypes to older versions for now because I could not solve all the issues and I did not want to further mess up things. It already took me a few hours to figure out what went wrong...I will try to have another look after the merge...

@tisto tisto merged commit a9cfd9c into master Apr 18, 2016
@tisto tisto deleted the lg-search branch April 18, 2016 10:54
@tisto tisto removed the in progress label Apr 18, 2016
@lukasgraf
Copy link
Member Author

@tisto no worries 😉 Let me know if I can help.

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

Successfully merging this pull request may close these issues.

None yet

6 participants