Skip to content

Fix Regression in generating queries with nested maps with numeric keys #3689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

dajulia3
Copy link
Contributor

@dajulia3 dajulia3 commented Jun 27, 2021

While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails.

Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value.

This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key.

This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach. There certainly is room to refactor this according to the maintainers' preference. I'm happy to take a pass at refactoring it to use just the index-based access to DRY things up a bit as long as the overall approach looks good. Or if it's quicker/easier for the maintainers to do the refactoring, no problem.. whatever gets this fix in quickest 😀.

Signed-off-by: David Julia [email protected]

This should fix #3688

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 27, 2021
@dajulia3 dajulia3 force-pushed the fix-nested-int-key-maps branch 4 times, most recently from 70f25b7 to 80f0215 Compare June 27, 2021 23:59
@christophstrobl christophstrobl self-assigned this Jun 28, 2021
@dajulia3 dajulia3 force-pushed the fix-nested-int-key-maps branch from 80f0215 to 8eb75ba Compare June 28, 2021 13:09
@christophstrobl
Copy link
Member

Thanks @dajulia3 putting this together.
Unfortunately this PR breaks multipleNumericKeysInNestedPath and numericKeyForMap in UpdateMapperUnitTests.
Care to have another look?

@christophstrobl christophstrobl added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 1, 2021
@dajulia3
Copy link
Contributor Author

dajulia3 commented Jul 2, 2021

Sure thing! I thought I ran all the tests but I must have missed running that one in the later commits. I'll take another look!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 2, 2021
While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails.

Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value.

This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key.

This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach. There certainly is room to refactor this according to the maintainers' preference.

fixes spring-projects#3688
@dajulia3 dajulia3 force-pushed the fix-nested-int-key-maps branch from 8eb75ba to d4bbed8 Compare July 5, 2021 03:01
@dajulia3
Copy link
Contributor Author

dajulia3 commented Jul 5, 2021

@christophstrobl those tests now pass for me locally. It's ready for a second look :)

christophstrobl pushed a commit that referenced this pull request Jul 6, 2021
While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails.

Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value.

This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key.

This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach.

Fixes: #3688
Original Pull Request: #3689
christophstrobl added a commit that referenced this pull request Jul 6, 2021
Simplify KeyMapper current property/index setup.

Original Pull Request: #3689
christophstrobl pushed a commit that referenced this pull request Jul 6, 2021
While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails.

Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value.

This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key.

This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach.

Fixes: #3688
Original Pull Request: #3689
christophstrobl added a commit that referenced this pull request Jul 6, 2021
Simplify KeyMapper current property/index setup.

Original Pull Request: #3689
christophstrobl pushed a commit that referenced this pull request Jul 6, 2021
While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails.

Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value.

This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key.

This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach.

Fixes: #3688
Original Pull Request: #3689
christophstrobl added a commit that referenced this pull request Jul 6, 2021
Simplify KeyMapper current property/index setup.

Original Pull Request: #3689
@christophstrobl
Copy link
Member

Thanks @dajulia3 for taking the time to update the PR.
Changes got merged to the main line and back ported to 3.2.x & 3.1.x branches.
We created #3697 to improve that area for the next major release.

@derekxia1988
Copy link

Thanks @dajulia3 for taking the time to update the PR. Changes got merged to the main line and back ported to 3.2.x & 3.1.x branches. We created #3697 to improve that area for the next major release.

Hi, do you have any idea to merge this fix to 3.0.x? Since springboot 2.3.12.RELEASE use spring-data-mongodb3.0.9, which has this problems too. Springboot 2.4 changes a lot and we cannot upgrate easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple maps with numeric keys in a single update produces the wrong query (Regression)
4 participants