Fix upsert too long id #103399
Fix upsert too long id #103399
Conversation
elasticsearchmachine
added
v8.13.0
needs:triage
pxsalehi
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
| @@ -495,6 +496,27 @@ public void testToValidateUpsertRequestWithVersion() { | |||
| assertThat(updateRequest.validate().validationErrors(), contains("can't provide version in upsert request")); | |||
| } | |||
|
|
|||
| public void testUpdatingRejectsLongIds() { | |||
| String id = randomAlphaOfLength(511); | |||
There was a problem hiding this comment.
@wdongyu this can be looped. code isnt compact here and DRY/iteration can be followed.
There was a problem hiding this comment.
@bohemia420 just follow the testIndexingRejectsLongIds func in IndexRequest, ok to be looped
| request.doc("{}", XContentType.JSON); | ||
| validate = request.validate(); | ||
| assertThat(validate, notNullValue()); | ||
| assertThat(validate.getMessage(), containsString("id [" + id + "] is too long, must be no longer than 512 bytes but was: 513")); |
There was a problem hiding this comment.
@wdongyu 512, 513 are hardcoded and repeated. 512 can be a maxlen constant.
|
@wdongyu there may be no need to have 2 commits - kindly squash into 1. |
|
is there any nomenclature for git branching? can we name it better than |
No, don't do this. Whenever this gets actually reviewed and merged, it will be squashed. Never force push with a squashed commit once a PR starts getting reviewed.
Branch names mean nothing. As long as the branch merger references the issue ID in the commit message, its fine. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@wdongyu I committed a change log entry for this change. |
|
buildkite test this |
benwtrent
added
:Distributed/CRUD
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
this pr lefts a task only, could you assign an assignee @benwtrent @bohemia420 |
|
I am pinging the experts on this part of the code. Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi, thanks for the PR! It looks reasonable. Initially I wondered whether we should consider this a breaking change or a bugfix, but after some discussion with the team, we're already documenting the 512 limit so we can consider this as a bugfix. Will rebase the branch, pass another final CI and will merge it afterwards. |
|
buildkite test this |
|
🎉 🎉 🎉 @wdongyu 🎉 🎉 🎉 Thank you for the contribution!!! |
|
Thank you all guys for the approval of the first pr from a beginner😊 |
Closes #102981