Data stream stats: Add name only if present

[DewaldV]

Description

This makes the GetDataStreamStats request only add the Name and path separator (/) to the path if Name is not empty.

As it stands when using this function without setting a name the path will be:

/_data_stream//_stats

This path is then used by the AWS Signer to calculate a signature. When the request is sent the server recalculates the signature but the additional / has been dropped by now and we get a signature mismatch. See #337 for details.

I have tested this fix in our own codebase and it resolves the issue as described.

Issues Resolved

Closes #337

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.

[dblock]

Looks good, care to add a test, please?

[VachaShah]

Thank you @DewaldV! +1 for adding a test for this.

[DewaldV]

Thanks for taking a look, I'll add a test to this as requested. 👍

Apologies for my delayed response I've been at conference this week.

[DewaldV]

@VachaShah @dblock apologies for the delay in getting those tests sorted but I've added them as requested.

The integration test is a bit strange since I had to add additional Data Stream create and delete tests so that I could ensure I was actually getting multiple results from the /_data_stream/_stats endpoint as expected. Not sure if you're prefer we split it out to a different test so let me know.

I added a unit test to specifically check the behaviour of setting the Name field of the IndicesGetDataStreamStatsRequest as that was causing my original issue. I felt the Integration Test didn't really cover that specifically so a unit test would be a good way to ensure the URL is valid.

[dblock]

This is perfect, my favorite kind of PR that has way more tests than an otherwise trivial fix!

[dblock]

@DewaldV see failures in tests + missing license header

[DewaldV]

@dblock Apologies that I missed that. Just pushed the license headers!

[dblock]

@dblock Apologies that I missed that. Just pushed the license headers!

Cool. I disabled the PR behavior to require approvals from maintainers to run for 1st time contributors (made it for new GitHub users only)

[DewaldV]

Ahh, the test failure I thought might be an issue but I ran a it a few times locally and it seemed OK so I left it as is.

I suspect the /_data_stream/_stats endpoint returns the data streams in any order.

When I tested locally it seemed consistent but it's likely consistent for my local cluster only compared to running it somewhere else.

I'll have to refactor that test to change how we do the assertion so its not order dependent.

[DewaldV]

Hey @dblock. 👋

After some research and attempts at fixing the ordering issue in tests I found the jsonassert library that tackles exactly these sorts of issues. It might be a very useful tool for other tests in this lib given the usage of JSON for all OpenSearch APIs.

It includes functionality for asserting JSON objects match without considering array order and should fix the flakiness in my original test.

I understand introducing a new dependency isn't ideal and might need more conversation but this is only used in Integration Tests at this point so hopefully it can be considered.

[DewaldV]

I've checked the failures on the latest CI run and I can't replicate them locally.

I'm convinced they are unrelated to my changes and tests and may just need a rerun to pass.

I can do some more digging if needed thoug, let me know what you think. 👍