prismicio / javascript-kit

Development kit for the Javascript language
https://developers.prismic.io
106 stars 69 forks source link

Error 500 when using form + predicate #3

Closed rudyrigot closed 10 years ago

rudyrigot commented 10 years ago

The JS kit test #6 fails because calling this: Api.forms('products').ref(Api.master()).query('[[:d = at(my.product.flavour, "Chocolate")]]').submit(callback); generates this URL with two q parameters (one brought by the form, one brought by the query): https://lesbonneschoses.prismic.io/api/documents/search?q=%5B%5B%3Ad%20%3D%20any(document.type%2C%20%5B%22product%22%5D)%5D%5D&q=%5B%5B%3Ad%20%3D%20at(my.product.flavour%2C%20%22Chocolate%22)%5D%5D&ref=UkL0hcuvzYUANCrm

The server responds with an error 500: {"message":"shouldhandlecaseswithseveralqueries!"}

How it happens: SearchForm.query() method ends up with a this.data.q which is an array of 2 values: [[:d = any(document.type, ["product"])]] and [[:d = at(my.product.flavour, "Chocolate")]] Later, the SearchForm.submit() method loops through these and adds one parameter per array item to the URL, therefore sending the two q parameters.

I wanted to fix this right (should I change the query() method or the submit() method?), so I went to see how it was done in Java, and I think I'm missing some information, because in the Java kit, this is handled within a private method called SearchForm.strip() that comes with a // Temporary hack for Backward compatibility comment, and does some string parsing. Is this how it should be done in JS as well for now?

guillaumebort commented 10 years ago

This must be fixed at the API side, several q parameters should be valid.

sadache commented 10 years ago

I pushed a quick fix for the validation which is the original cause. Now we need to handle errors in multiple queries (this was hiding the real error), and we should fix the browser which doesn't expect to receive the current response format (API browser is broken when multiple q are used)

On Sat, Nov 23, 2013 at 8:46 AM, Guillaume Bort notifications@github.comwrote:

This must be fixed at the API side, several q parameters should be valid.

— Reply to this email directly or view it on GitHubhttps://github.com/prismicio/javascript-kit/issues/3#issuecomment-29127948 .

http://sadache.tumblr.com ʎdoɹʇuǝ

rudyrigot commented 10 years ago

Ok, can we leave this issue open so I remember to test it in JS when it's fixed in the API, and to let the user know about it?

So, this means there's actually no bug in the JS kit, and that there was a small one in the Ruby kit, that Etienne already fixed today.

sadache commented 10 years ago

Actually it should be fixed now, can you test it? It is only the ApiBrowser that needs to fixed (and we need to fix reporting errors in multiple queries)

On Sat, Nov 23, 2013 at 8:06 PM, Rudy Rigot notifications@github.comwrote:

Ok, can we leave this issue open so I remember to test it in JS when it's fixed in the API, and to let the user know about it?

So, this means there's actually no bug in the JS kit, and that there was a small one in the Ruby kit, that Etienne already fixed todayhttps://github.com/prismicio/ruby-kit/commit/7c422ef34e06d210372e918f0badbd99a273c985 .

— Reply to this email directly or view it on GitHubhttps://github.com/prismicio/javascript-kit/issues/3#issuecomment-29139110 .

http://sadache.tumblr.com ʎdoɹʇuǝ

rudyrigot commented 10 years ago

Actually, never mind, the correction you made did fix the test; let me know when it's pushed to npm so I can let the user know.

rudyrigot commented 10 years ago

Yes, exactly.

rudyrigot commented 10 years ago

Wait, there's nothing to push to npm... argh, not entirely awake... I'll let the user know!

Close this if you want.

rudyrigot commented 10 years ago

This shouldn't be here, rather in the product's repo, I'll close it for now; let me know if you want me to reopen it.