Error 500 when using form + predicate #3

Closed
rudyrigot opened this Issue Nov 22, 2013 · 8 comments

Comments

Projects
None yet
3 participants
@rudyrigot
Contributor

rudyrigot commented Nov 22, 2013

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

This comment has been minimized.

Show comment Hide comment
@guillaumebort

guillaumebort Nov 23, 2013

Contributor

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

Contributor

guillaumebort commented Nov 23, 2013

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

@sadache

This comment has been minimized.

Show comment Hide comment
@sadache

sadache Nov 23, 2013

Contributor

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ǝ

Contributor

sadache commented Nov 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@rudyrigot

rudyrigot Nov 23, 2013

Contributor

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.

Contributor

rudyrigot commented Nov 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@sadache

sadache Nov 23, 2013

Contributor

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ǝ

Contributor

sadache commented Nov 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@rudyrigot

rudyrigot Nov 23, 2013

Contributor

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.

Contributor

rudyrigot commented Nov 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@rudyrigot

rudyrigot Nov 23, 2013

Contributor

Yes, exactly.

Contributor

rudyrigot commented Nov 23, 2013

Yes, exactly.

@rudyrigot

This comment has been minimized.

Show comment Hide comment
@rudyrigot

rudyrigot Nov 23, 2013

Contributor

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

Close this if you want.

Contributor

rudyrigot commented Nov 23, 2013

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

Close this if you want.

@rudyrigot

This comment has been minimized.

Show comment Hide comment
@rudyrigot

rudyrigot Dec 6, 2013

Contributor

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.

Contributor

rudyrigot commented Dec 6, 2013

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.

@rudyrigot rudyrigot closed this Dec 6, 2013

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