Enhance the vocabularies serializer to accept a list of tokens

[sneridagh]

We will need this next week.

[mister-roboto]

@sneridagh 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!

[tiberiuichim]

May I suggest tokens instead of token_list as parameter? Also, I think we'd need to have a POST service here: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/vocabularies/configure.zcml

[sneridagh]

I test drive it, and it needs adjustments. Do not merge yet.

@tiberiuichim Better not POST for caching, right? Also token and tokens, wouldn't it be far too similar and error prone?

[sneridagh]

@jenkins-plone-org please run jobs

[tisto]

We should make a decision regarding tokens vs token_list and add it to our naming convention:

https://github.com/plone/plone.restapi/blob/master/docs/source/conventions.rst

I will do some research...

[sneridagh]

@jenkins-plone-org please run jobs

[sneridagh]

@tisto @tiberiuichim ready. Used tokens as parameter. Sticked to the GET request for caching purposes.

[tiberiuichim]

As far as I understand, we're not gonna be able to have tokens with commas in them, right? I don't know how common this use case is.

[sneridagh]

@tiberiuichim you have a point, ideally not... but who knows... could be a vector of bugs.

The whole "let's decide how we encode lists in parameters" story is also important, I just used what we already have in there.

[ksuess]

Same topic, how to handle list parameters, in search endpoint.
? Is it a default for a JSON API to handle commas as list separators?
search endpoint gets
@@search?metadata_fields:list=subject,modified&portal_type:list=News Item,Event
then searches for
query {'metadata_fields': ['subject,modified'], 'portal_type': ['News Item,Event']}
which returns an empty result.

[tiberiuichim]

that list argument conversion is not correct. https://zope.readthedocs.io/en/latest/zdgbook/ObjectPublishing.html#argument-conversion.

list – collect all values with this name into a list.

It should be @search?metadata_fields:list=subject&metadata_fields:list=modified

[sneridagh]

@ksuess in the search endpoint we are constrained by how Zope Querystring parser search parameters are encoded so the action is taking already care of passing Zope the right way (see the action code) in jsx, when you call the action, just pass a JS array in the action.

[sneridagh]

hehe, @tiberiuichim was faster than me :)

[tisto]

This PR raises a number of REST API design principles questions:

1. Do we want to use multiple parameters dependent on if we pass a single parameter (e.g. "token=foo") or a list of parameters (e.g. "tokens=foo,bar"). Some APIs use a single parameter, no matter if you pass a single parameter or multiple parameters (e.g. "token=foo" and "token=foo,bar"). I think I personally prefer to use a param that accepts both a single term and a list.
2. Do we want to include the type of the passed param into our param name (token vs token_list). I'd be ok with the plural solution that we currently have in this PR. Including the type might be a slippery slope.
3. URL encoding lists. There are a bazillion ways of handling this. You can just urlencode stuff (like "foo,bar" -> "foo%82bar"). You can pass a parameter multiple times (e.g. token=foo&token=bar). You can encode stuff like "[foo,bar]", ...

I started to dip my toe into urlencoding arrays and nested structures for the querysting-search endpoint (#1252 (comment)) and I have to admit it is just a horrible mess and there is no standard whatsoever as far as I can see.

[tiberiuichim]

@tisto There's also the option of using Zope's argument parsing, with tokens:list=foo&tokens:list=bar

[ale-rt]

My 2 c., do what we are used to do!

Either use:

• tokens=foo&tokens=bar
  or:
• tokens:list=foo&tokens:list=bar
  if you want to be explicit.

Notes:

1. while there is no difference when the parameter appears once in the query string, id does make a difference when it is present just once, i.e. token=foo will be give you a self.request.form equal to {"token": "foo"}, while token:list=foo will give you a self.request.form equal to {"token": ["foo"]} (notice the list).
2. If you opt for tokens=foo,bar you must protect your code from people passing the parameter multiple times, i.e.: tokens=foo,bar&tokens=baz.
3. Just a curiosity, query string can be exploited and Zope does a great job in preventing that. There is tons of stuff to read, start from https://en.wikipedia.org/wiki/HTTP_parameter_pollution. IIRC also facebook was found vulnerable to this attack (PHP...)

[jensens]

I agree with @ale-rt - and I tend to not reinvent the wheel and use Zope list query parameter syntax.

[sverbois]

just my 2c

When I design REST API I always use a plural noun when the API client can provide mutliple values. It is more predictive for the user.
Here I would use:
tokens:list=foofor the singleton list tokens=['foo']
tokens:list=foo&tokens:list=bar for a multi-valued list tokens=['foo','bar']

[sneridagh]

@jenkins-plone-org please run jobs

[sneridagh]

@ale-rt @jensens refactored to use multi valued, as in Zope. I will merge if all is good from you!

[ale-rt]

Mine was just a suggestion :)
I am good with this PR.
I would wait for the reviews of @tisto and @ericof ;)

[sneridagh]

@ale-rt a good one! Thanks for weigh in! We already talked about it in today's Volto Team meeting and agreed to go this way.

[tisto]

I am good with this PR. ToDo:

• Add the multiple params approach to API design best practices
• Add test and docs to show that you can pass a single param with "tokens" as well
• Deprecate "token" for the future
• Refactor usage of other approaches (e.g. "," in expendables) to support the new "standard", deprecate the old way for the next major release

[ale-rt]

Given that you are into json you might also be interested in making this happen: zopefoundation/Zope#957