fix query parameter name generator in azure-spring-data-cosmos #23341
Conversation
Replace any non-alphanumeric characters in the generated query parameter name with an underscore so that criteria with references to nested properties such as user['first name'] do not result in a bad request due to invalid query parameter name. Resolves Azure#23084
gogopavl
requested review from
backwind1233,
chenrujun,
kushagraThapar,
mbhaskar,
saragluna,
stliu and
yiliuTo
as code owners
ghost
added
Cosmos
azure-spring
|
Thank you for your contribution gogopavl! We will review the pull request and get back to you soon. |
|
|
|
@mbhaskar - can you please take a look at this PR ? |
...cosmos/src/main/java/com/azure/spring/data/cosmos/core/generator/AbstractQueryGenerator.java
Show resolved
Hide resolved
| @@ -567,6 +567,19 @@ public void testBetweenCriteria() { | |||
| assertThat(people).containsExactly(TEST_PERSON); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Can you please come up with some more test cases, so that we can avoid regression for this in future ?
For example, you can add few test cases for ReactiveCosmosTemplateIT in addition to CosmosTemplateIT.
We generally prefer to add test cases for both sync and async APIs.
There was a problem hiding this comment.
@kushagraThapar sure, will do!
Initially, I was a bit sceptical to add more because of the overhead incurred by integration tests. I was also considering adding unit tests but saw that there's a preference for integration tests throughout the project.
There was a problem hiding this comment.
@gogopavl yes, that would be great.
There was a problem hiding this comment.
@kushagraThapar added 808d371
gogopavl
requested review from
michaelqi793,
moarychan,
zhichengliu12581 and
ZhuXiaoBing-cn
as code owners
|
@gogopavl - thank you, I will review the code. Meanwhile, I see CI is failing, I think because of some compilation issues. Can you please take a look and fix those ? |
Assert the change in the behaviour of the query parameter name generator by adding more tests using the reactive cosmos template References Azure#23084
gogopavl
force-pushed
the
bugfix/23084-invalid-parameter-name-for-nested-property
branch
from
808d371 to
5399af2
Compare
@kushagraThapar updated. Looks like two pipelines failed but the error doesn't seem specific to my changes. Noticed that the Cosmos DB Emulator didn't start in those instances: |
|
@gogopavl - thanks for fixing other issues, I will run the CIs and will let you know in case there are test failures. |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/check-enforcer override |
|
@gogopavl - I have merged this PR, it will get released in our next release. (September 2nd week). |
|
@kushagraThapar thank you too for helping out in the process & for your valuable feedback. Looking forward to the release! |
Replace any non-alphanumeric characters in the generated query parameter name with an underscore
so that criteria with references to nested properties such as
user['first name']do not resultin a bad request due to invalid query parameter name.
Resolves #23084