[FEATURE] Support for script operations in the update action of the client.helpers.bulk method

[huuyafwww]

Signed-off-by: huuyafwww [email protected]

Description

This is a modification to the client.helpers.bulk method to support script operations in the update action.

Issues Resolved

#209

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[huuyafwww]

Here is the bulkBody of the API request after and before the modification.
This modification makes the bulkBody I wanted!

sample code here:

const body = [[
  { update: { _id: '1', _index: 'sample-index'} },
  { script: {
      source: "ctx._source.body = params.body;",
      lang: 'painless',
      params: {
        body: "sample body",
      }
    },
    scripted_upsert: true,
  },
]];

await client.helpers.bulk({
  datasource: body,
  onDocument (doc) {
    return doc;
  }
});

before:

{"update":{ "_id": "1", "_index": "sample-index"}},
{"doc":[{"update":{ "_id": "1", "_index": "sample-index"}},{"script":{"source":"ctx._source.body = params.body;","lang":"painless","params":{"body":"sample body"}},"scripted_upsert":true} 

after:

{"update":{ "_id": "1", "_index": "sample-index"}},
{"script":{"source":"ctx._source.body = params.body;","lang":"painless","params":{"body":"sample body"}},"scripted_upsert":true}
// ↑ Even doc operations generate the expected bulkBody.

[kavilla]

Hello @huuyafwww, thanks for opening this! Since this changes a request, I would consider this a major version change which could impact other consumers of the package. To build this in a way that makes it possible to get your desired behavior by making it configurable with the default path being the current request.

[huuyafwww]

Hello @kavilla , Thanks for the check.

Since this changes a request, I would consider this a major version change which could impact other consumers of the package.

Indeed it is.
I agree with you.

To build this in a way that makes it possible to get your desired behavior by making it configurable with the default path being the current request.

Ok.
Am I correct in assuming that we build a "doc" or "script" depending on the contents of the chunk or action?

[huuyafwww]

@kavilla

Am I correct in assuming that we build a "doc" or "script" depending on the contents of the chunk or action?

This is fine, but if you can determine that the operation is a "doc" or "update" operation by the code below, for example, you know that the developer has entered "doc" or "update" as the return value of onDocument in the first place.

const document = Array.isArray(action)
  ? Object.keys(action[1])[0]
  : Object.keys(action)[0]

So we assigned action[1] as the completion of Object.keys(action[1])[0] and chunk as the completion of Object.keys(action)[0].

payloadBody = serializer.serialize(action[1] || chunk)

What do you think about my idea?

[huuyafwww]

Hi, @kavilla!
What do you think of my opinion?
Thanks for taking the time to respond.

[kavilla]

Hello @huuyafwww, will take a look next week if that is okay! Sorry about the delay.

[huuyafwww]

Hello @kavilla, Thanks again for your thoughtful replies.
Understood!

[kavilla]

Sorry for the delay, I was incapacitated for the past two weeks! I see what you are getting at and makes sense for me. But since having forked this repo I will need to derive the same level of insight as you. So what you are saying is that by the time the update operation is hit there was essentially no reason why it was checking if it was a doc or not since the value should be correct and that we were previously spreading the completion into the payload body which was the root case of script operations not being supported in update operations. Because basically it was getting incorrectly applied everytime?

[huuyafwww]

Sorry for the delay, I was incapacitated for the past two weeks!

Don't worry about it.
Thank you for confirming my request in spite of all that.

But since having forked this repo I will need to derive the same level of insight as you.

Roger that!

So what you are saying is that by the time the update operation is hit there was essentially no reason why it was checking if it was a doc or not since the value should be correct and that we were previously spreading the completion into the payload body which was the root case of script operations not being supported in update operations. Because basically it was getting incorrectly applied everytime?

You're right!
There are doc and script for bulk api update operations, but only doc was supported by this helpers.bulk in the first place.
The implementation was based on the doc premise and was designed to be complementary, so there was no way to request an update operation in the script.
Therefore, I am modifying the relevant section in this pull request to also support script updates in this helpers.bulk.

Also, as you can see, there was no need for a conventional complementary implementation in the first place.
This is because if this library can determine whether the update operation the developer is trying to perform is doc or update, there is no need for completion in the first place.

In addition, to explain, being able to determine whether the payload body of an update operation is doc or update is no different from receiving that information directly from the return value of onDocument.

Therefore, since it is no longer necessary to supplement the payload body as before, we have modified it in this way because we believe that the best implementation is to use the return value given by the developer using this library as-is as the payload body.

[huuyafwww]

@kavilla
Hi kavilla.
How about what I am proposing in this pull request?

I have already patched this pull request change to the business system and am running it in the production environment.
If possible, I would like to see it taken in.

[huuyafwww]

Since this pull request was created, the contents of the main branch had changed and was no longer working with the latest version, so the main branch was imported and modified.

[dblock]

Thanks. Needs tests for UpdateActionScriptOperation and a changelog entry.

[huuyafwww]

@dblock

Thanks. Needs tests for UpdateActionScriptOperation and a changelog entry.

Sorry for the delay in responding.
I tested UpdateActionScriptOperation and added an entry to the changelog.
I also added a bulk update test for script operations.

[kavilla]

lgtm. Any issues?

[huuyafwww]

@kavilla

lgtm. Any issues?

thanks.
I can no longer bulk update doc operations in the conventional way,
It involves some disruptive changes.

[wbeckler]

@nhtruong would you be willing to take a look, especially with respect to backwards compatibility?

[nhtruong]

Is this the reason why this breaks the bulk operation for regular use of this helper method?

(Also sorry the comment mentioning me got lost in the mail)

[huuyafwww]

@nhtruong
Yes. The old method only supported doc, so we modified it to support script as well, but that will not work with the previous method of input.

[nhtruong]

We should avoid breaking changes for existing users. Unless there are no other ways around this?

[nhtruong]

@huuyafwww have you found a work around to avoid breaking current use cases?

[dblock]

Looks like we failed to merge this after all. @nhtruong could you please take a look?

[Catch All Triage - Attendees 1, 2, 3, 4, 5]