search

typesense / typesense-go

Go client for Typesense: https://github.com/typesense/typesense
Apache License 2.0
222 stars 56 forks source link

Fix URL encoding for FilterBy #77

Closed khalilsarwari closed 3 years ago

khalilsarwari commented 3 years ago

Change Summary

The current parsing assumes it has been encoded/escaped already, so this change adds escaping. Seems to resolve #71 , though perhaps this file really shouldn't be touched and it should be done some other way?

PR Checklist

kishorenc commented 3 years ago

@khalilsarwari

Thank you for your contribution!

perhaps this file really shouldn't be touched

Yes, we cannot edit generated files.

it should be done some other way?

We can use the Transport parameter to do this as shown in this example. The parameter can be added to this line.

cc @v-byte-cpu

khalilsarwari commented 3 years ago

Thanks for the pointers @kishorenc, I'll be sure not to edit the generated files then. I tried adding a transport wrapper at the line you suggested, but it comes too late I believe. The request url given as input to the transport wrapper is already deformed due to the parsing error taking place earlier.

For example, when I add the transport wrapper, the request URL is: /search?+label_names%3A=%5Bnews%5D&filter_by=created_at%3A%3E%3D300

where the intended filter by is created_at:>=300 && label_names:=[news]

The start of the block here has the correct value for filter by, and by the end of that if block, the query values are incorrectly added. This all takes place before reaching the transport wrapper I added at the line you mentioned.

Assuming we want *api.SearchCollectionParams to remain as is and not require the user to specify the filter in an encoded way, there needs to be an encoding step between *api.SearchCollectionParams being created and the aforementioned block being reached. If this is the case, I am not sure where the best place would be. Perhaps the line before here there could be a reformatting step where filter by is encoded?

kishorenc commented 3 years ago

@khalilsarwari

This all takes place before reaching the transport wrapper I added at the line you mentioned.

Got it.

Perhaps the line before here there could be a reformatting step where filter by is encoded?

Yes, I think this is a reasonable solution. Let's go ahead with that! We might have do the encoding for other parameters as well which might be affected (e.g sort_by).

khalilsarwari commented 3 years ago

I've added the encoding for filter_by and q for now, for the other fields it seems unlikely that they would have a character that needs to be escaped (having a colon : already is supported).

kishorenc commented 3 years ago

LGTM @khalilsarwari -- can we also add this test which was earlier used to catch this issue?

https://github.com/typesense/typesense-go/pull/63/files#diff-64156d1456080451115f1fecb9da1edd93a51029f5f8dd0f4693279741a024a0R60

Any suggestions/comments @v-byte-cpu?

v-byte-cpu commented 3 years ago

@kishorenc yes, we should add the mentioned test to check that the original issue was fixed.

khalilsarwari commented 3 years ago

I have added the test @kishorenc @v-byte-cpu (please let me know if this should have been done another way, or if there is anything else)

kishorenc commented 3 years ago

@khalilsarwari Looks great. My final question: is there a reason for adding the query escaping logic to the tests here: https://github.com/typesense/typesense-go/pull/77/commits/0335f5a43ee51d5bf4aca8253a9a9272b1080f61

Were the tests failing without this change?

khalilsarwari commented 3 years ago

Yes, the tests were failing without this change. The changes made in this PR to typesense/documents.go are not reached/executed by the mockAPIClient, yet this is the earliest place that I think we could make the change.