Fix script upsert on bulk requests

[dsgrillo]

According to the documentation, it's required to add the scripted_upsert=true for running a upsert with script.
If the flag is not set, the document is added, but without running the script.

I've added a test that replicates the issue and then fixed it.

[ruflin]

@dsgrillo Thanks for the contribution. Could you add a bit more description to your PR on what the issue is and how it is fixed?

[deguif]

Here's what the documentation states about it regarding the link you posted.

Upsert

If the document does not already exist, the contents of the upsert element are inserted as a new document. If the document exists, the script is executed:

Scripted upsert

To run the script whether or not the document exists, set scripted_upsert to true:

[dsgrillo]

Hi @deguif, thanks for making it clearer! 😄

In the context of upsert, scripts are usually used for making a partial update in the document, thus is usually not suitable to only use document for the insert, but start with a an "empty" and run the script for updating it to the desired state. Otherwise, there's a chance for a race condition and you might end up loosing one of the updates.

[ruflin]

To keep the option option to also set it to false I wonder if we should pass a second param to the setScript method.

[dsgrillo]

Problem is that the setScript is called in the AbstractDocument. Don't think it makes much sense to mix the document and script here (in this case, adding one specific parameter of script).

Maybe adding it in the AbstractScript would be better?
So it becomes somehow similar to the implementation of doc_as_upsert.

[ruflin]

If we add it to AbstractScript, would it be also useful to the other classes or it doesn't make sense there?

Didn't fully get your comment related to doc_as_upsert. Can you share more?

[dsgrillo]

Well, the AbstractScript is used in other places, not only for bulk operations. I didn't reviewed all the cases, but I'm pretty confident that the scripted_upsert won't be expected for all of them. Having said that, we'd be using that parameter only within the bulk context.

What I meant with the doc_as_upsert is regarding the implementation above (lines 18-27 of the same file), where we check for the value of $document->getDocAsUpsert() and only then add it to the request.

[ruflin]

@dsgrillo Sorry that this got stalled on my end. As you describe above, adding it to AbstractScript might not be ideal as it is not expected for all of them. So we are left with how to add it here.

What you propose with $document->getDocAsUpsert() sounds like a good path forward. If I understand this correct it means it would get the "value" from the doc itself and we would not need an additional param? I guess this requires that the document is set first?

[ruflin]

@dsgrillo Thank you for the contribution and sorry that it took so long to get this reviewed on my end. Somehow I missed that 21 days ago there were additional commits 🤦‍♂️