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

Update RequestConverters.java #36988

Closed
wants to merge 1 commit into from
Closed

Update RequestConverters.java #36988

wants to merge 1 commit into from

Conversation

Kingsea442
Copy link

fix exception when _id is like this /a/b/c

code:
UpdateRequest updateRequest = new UpdateRequest("dev_product","product", "/sku/test.com/48923AQ3");
// ignore
restHighLevelClient.update(updateRequest, RequestOptions.DEFAULT);
exception:
[dev_product/6rBlaK6USlaQ5frl4HGmWg][[dev_product][0]] ElasticsearchStatusException[Elasticsearch exception [type=document_missing_exception, reason=[product][test.com/48923AQ3]: document missing]

URI [/dev_product/product/test.com%2F48923AQ3/_update?timeout=1m], status line [HTTP/1.1 404 Not Found]

the exception is convert this id /sku/test.com/48923AQ3 to test.com%2F48923AQ3

fix exception when es _id like this /a/b/c
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@jakelandis
Copy link
Contributor

@Kingsea442 - Can you please add unit tests. Specifically ones that ensure this comment is still valid:

//we prepend "/" to the path part to make this path absolute, otherwise there can be issues with
//paths that start with - or contain :

@javanna
Copy link
Member

javanna commented Jan 2, 2019

hi @Kingsea442 ,
your fix is causing some failures in RequestConvertersTests. I do see the problem you are having though and I will look into how we can fix it.

@Kingsea442
Copy link
Author

hi @Kingsea442 ,
your fix is causing some failures in RequestConvertersTests. I do see the problem you are having though and I will look into how we can fix it.

great! thanks.

@javanna
Copy link
Member

javanna commented Jan 14, 2019

Closing in favour of #37405 which I just opened. I did not have time to work on a fix yet but I have added a failing test to the linked issue, which shows the problem.

@javanna javanna closed this Jan 14, 2019
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

Successfully merging this pull request may close these issues.

5 participants