Add support for Search Option "seq_no_primary_term" and Index Options…

[ryu818]

… "if_primary_term" and "if_seq_no"

[ryu818]

This change addresses issue #2232. Initially, I tried to create options using $endpoint->getParamWhitelist(), but I realized that the options are not only for the endpoint. Therefore, I made changes to add to the existing ones, minimizing the impact. Please review the changes.

[ruflin]

@ryu818 Really sorry for the late reply. I just started CI on it. Any chance you could add a line to the changelog?

[ryu818]

I have also updated the changelog. However, it seems there are some errors in the CI. I do not have a test execution environment and was unsure how to fix them. Could you please assist me with this?

[ruflin]

Ideally we would have some tests that test the changes. I will try to have a look if the test failures are related to this change or not. My guess it is as we just had recently a clean CI pass. About the changelog, a release went just out a few hours ago so we have a conflict now. Let me know if you want to resolve it otherwise habe to help with it.

For the test suite, do you have phpunit installed? If yes, that is step one. Then to get elasticsearch running, there is a docker compose setup, here some more details how Github runs it: https://github.com/ruflin/Elastica/blob/8.x/.github/workflows/continuous-integration.yaml#L114

[ruflin]

BTW this is the failure I'm worried about:

1) Elastica\Test\IndexTest::testAddDocumentAcrossIndices
Elastic\Elasticsearch\Exception\ClientResponseException: 400 Bad Request: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"},"status":400}

It seems the injection of ifSeqNo caused an error

[ryu818]

Thank you for your response. The conflict has been resolved. I am running elasticsearch8, php8.1 and phpunit on a development server for another project, not in a docker environment. I tried to test it with “php vendor/bin/phpunit --filter IndexTest” and so on, but it does not return any response as if there is not enough memory.

1) Elastica\Test\IndexTest::testAddDocumentAcrossIndices
Elastic\Elasticsearch\Exception\ClientResponseException: 400 Bad Request: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"},"status":400}

This error occurs when adding a document to Elasticsearch because if_seq_no is not set while if_primary_term is set. Elasticsearch expects both if_seq_no and if_primary_term to be set.

I think it would be better to fix the part where primary_term is set by default in the document when adding it. I suspect that if_primary_term is being set despite if_seq_no not being included in the response from Elasticsearch in setVersionParams.

Also, to test if if_seq_no and primary_term are working correctly, it would be sufficient to add a document that was retrieved using getDocument. (We also have individual setSequenceNumber and setPrimaryTerm methods available, so we could use those as well.)

[ruflin]

The proposed change sounds sensible to me. Do you want to try to implement it that way and add a test?

For the changelog: It seems your editor "adjusted" some of the indentation. Any change you could revert the indentation part to not have a massive diff in the changelog?

[ryu818]

Regarding the indentation in the ChangeLog, I apologize for the inconvenience. I have corrected it again, could you please check if it is okay now?

Also, regarding the addition of tests, I apologize for the trouble, but could you please take care of it?

[ruflin]

Hi @ryu818 Sorry for the late reply. At the moment I don't get to look at it in much detail but it is on my list to have a look eventually. I also started the tests again to see what the output is.

[ryu818]

Hello. Never mind. Sorry for the delay in confirming this as well. This modification caused one error in the test, so we have also corrected the test code.

The _primary_term and _seq_no were taken into account before, and getDocument automatically sets if_primary_term and if_seq_no. However, in addDocument, if_primary_term, if_seq_no were not allowed and were ignored. As a result of enabling them, thestAddDocumentAcrossIndices now has an error. This is because it tried to add index2 using if_primary_term, if_seq_no in index1. Please could you check again?

[ryu818]

"This is a scheduled Ubuntu 20.04 retirement. Ubuntu 20.04 LTS runner will be removed on 2025-04-15. For more details, see https://github.com/actions/runner-images/issues/11101” and the test will be skipped. In “integration.yaml”, please correct “runs-on: ‘ubuntu-20.04’” to something like “runs-on: ‘ubuntu-22.04’”.

• https://github.com/ruflin/Elastica/blob/8.x/.github/workflows/continuous-integration.yaml#L5
• https://github.com/ruflin/Elastica/blob/8.x/.github/workflows/continuous-integration.yaml#L28
• https://github.com/ruflin/Elastica/blob/8.x/.github/workflows/continuous-integration.yaml#L136

[ruflin]

Have a PR with a potential fix here: https://github.com/ruflin/Elastica/pull/2243

[ruflin]

@ryu818 Can you rebase this either on 8.x or 9.x to continue? Tests should be green now.

[ruflin]

Hm, it seems the change is not visible in this PR anymore, I only see a diff for a test change? Did we get this in somewhere else?