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

Fix @search GET #274

Closed
wants to merge 2 commits into from
Closed

Fix @search GET #274

wants to merge 2 commits into from

Conversation

cdevienne
Copy link
Contributor

@cdevienne cdevienne commented May 27, 2018

The @search GET implementation does not json-decode the "q" parameter, which makes it impossible to use.

@coveralls
Copy link

coveralls commented May 28, 2018

Coverage Status

Coverage decreased (-0.006%) to 89.977% when pulling 27219bb on orus-io:fix-@search-get into c2ac0b3 on plone:2.5.x.

Copy link
Member

@vangheem vangheem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should then also change the permission from guillotina.SearchContent to guillotina.RawSearchContent

@cdevienne
Copy link
Contributor Author

I guess it makes sens. But if we are to make a difference between a raw search (which depends on the search engine if I understand correctly) and a search that would be engine-agnostic (with a common query language), how about renaming @search to @rawsearch ?

@vangheem
Copy link
Member

The GET endpoint was original designed to be this but implementation was never finished.

We can rename both POST and GET @rawsearch then too.

This is underdeveloped because we just don't use it. We have our own query builder and our own endpoint for doing querying.

I'm okay with negotiating solutions here. I just can't allow us to give reuse the guillotina.SearchContent here.

@cdevienne
Copy link
Contributor Author

Will do that then.

Now that the query is passed directly to the search engine, the permission
should be RawSearchContent, not SearchContent.
@cdevienne
Copy link
Contributor Author

I chose not to rename the service to @rawsearch because it would be an api breakage (hence not suitable for a maintenance release).

@vangheem
Copy link
Member

Sorry, looking over this more, the correct functionality would be to connect it with this method: https://github.com/guillotinaweb/guillotina_elasticsearch/blob/master/guillotina_elasticsearch/utility.py#L144

https://github.com/plone/guillotina/blob/master/guillotina/catalog/catalog.py#L35

I don't think we should make assumptions like q = json.loads(q) if we're going to try and implement something generic.

If you need this, the endpoint is simple enough, I suggest implementing it in your add-on code.

We'd probably need to think about some generic query language to support.

@cdevienne
Copy link
Contributor Author

Fair enough.
I will add my own endpoint then.

@vangheem vangheem closed this Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants