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

DocId needs url-encoding for certain operations #247

Open
robinp opened this issue Dec 3, 2018 · 7 comments
Open

DocId needs url-encoding for certain operations #247

robinp opened this issue Dec 3, 2018 · 7 comments

Comments

@robinp
Copy link
Contributor

robinp commented Dec 3, 2018

Creating a document with DocId containing slashes works, but fetching it with getDocument / documentExists doesn't find it. The difference is that creation uses JSON in POST, while the get/exist uses GET and url parameters.

So for the GET case, the DocId should be encoded.

I found #38 which seemed to be similar. I tried running the tests locally, but many of them failed anyway. Is there a special requirement for setting up a test env? (I have ES 6.5 running locally).

@bitemyapp
Copy link
Owner

Is there a special requirement for setting up a test env? (I have ES 6.5 running locally).

Yes, running ES 5.x >:P

https://github.com/bitemyapp/bloodhound/tree/master/src/Database we haven't ported to 6.x yet. Tracking Elastic's churn is labor intensive and we don't have a means of abstracting over the commonalities and differences that we're happy with yet.

Happily take a patch that fixes this url-encoding issue.

@robinp
Copy link
Contributor Author

robinp commented Dec 3, 2018

Good to know. Thanks, will have a look. By the way, BloodHound works with 6.3, at least for the functionality I was concerned so far.

@bitemyapp
Copy link
Owner

By the way, BloodHound works with 6.3, at least for the functionality I was concerned so far.

I'm not surprised, but I define compatibility as encompassing the entire test suite.

Good hunting!

@robinp
Copy link
Contributor Author

robinp commented Dec 5, 2018

Thanks. What would be the way to add support for 6.x? Copy the V5 code into a V6 module, and work incrementally?

@bitemyapp
Copy link
Owner

@robinp that's probably the most straight-forward way to go about it. @MichaelXavier sound copacetic to you?

@MichaelXavier
Copy link
Collaborator

That'd be how I'd do it!

@JoseD92
Copy link
Contributor

JoseD92 commented Oct 30, 2019

I tried to reproduce this issue but could not find a combination that allows me to insert/index a document but not read it, both indexDocument and getDocument use the DocId on url parameters, so it is always going to find whatever we can insert with indexDocument. @robinp do can you provide some example that shows how to reproduce the error.

As a side note, I create a twitter index as the examples seen in Database.V5.Bloodhound.Client using ES version 5.6.16 and bloodhound master branch on commit fc310b23c3c1698ff7a5ca3c54236ef84846dbf3, then insert and retrieve various documents with different names, I tried let myDoc = DocId s in (runBH' $ indexDocument testIndex testMapping defaultIndexDocumentSettings exampleTweet myDoc >> getDocument testIndex testMapping myDoc) with the values for s:

  • helloworld - inserts as expected and retrieves as expected.
  • hello world - inserts as expected and retrieves both hello world and hello%20world
  • hello%20world - inserts hello world and retrieves both hello world and hello%20world
  • hello/world - can't insert and can't retrieve
  • hello%2Fworld - inserts hello/world and retrieves hello/world

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

No branches or pull requests

4 participants