Skip to content
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

Use StringUtils.replace(…) instead of String.replaceAll(…) for mapKeyDotReplacement #3613

Closed
chzhong opened this issue Mar 29, 2021 · 4 comments
Assignees
Labels
type: enhancement A general enhancement

Comments

@chzhong
Copy link

chzhong commented Mar 29, 2021

When configured to use mapKeyDotReplacement, MappingMongoConverter will use String.replaceAll to replace all dots in keys into specified replacement, which might cause unnecessary creation of java.util.regex.Pattern repeatly. This will lead to rapid GC serialize/deserialize a huge (>= 10MB) mongo document multiple times.

Codes in current version:

	/**
	 * Potentially replaces dots in the given map key with the configured map key replacement if configured or aborts
	 * conversion if none is configured.
	 *
	 * @see #setMapKeyDotReplacement(String)
	 * @param source
	 * @return
	 */
	protected String potentiallyEscapeMapKey(String source) {

		if (!source.contains(".")) {
			return source;
		}

		if (mapKeyDotReplacement == null) {
			throw new MappingException(String.format(
					"Map key %s contains dots but no replacement was configured! Make "
							+ "sure map keys don't contain dots in the first place or configure an appropriate replacement!",
					source));
		}

		return source.replaceAll("\\.", mapKeyDotReplacement);
	}


	/**
	 * Translates the map key replacements in the given key just read with a dot in case a map key replacement has been
	 * configured.
	 *
	 * @param source
	 * @return
	 */
	protected String potentiallyUnescapeMapKey(String source) {
		return mapKeyDotReplacement == null ? source : source.replaceAll(mapKeyDotReplacement, "\\.");
	}

Suggestion:
Use org.springframework.util.StringUtils#replace or org.apache.commons.lang3.StringUtils#replace(java.lang.String, java.lang.String, java.lang.String) to replace dots instead.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 29, 2021
@mp911de
Copy link
Member

mp911de commented Mar 29, 2021

Good catch. Care to submit a pull request that makes use of org.springframework.util.StringUtils#replace?

@mp911de mp911de added status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 29, 2021
@chzhong
Copy link
Author

chzhong commented Mar 30, 2021

Good catch. Care to submit a pull request that makes use of org.springframework.util.StringUtils#replace?

I'm behind a firewall so I can't submit a PR :-(

@mp911de
Copy link
Member

mp911de commented Mar 30, 2021

Thanks for letting us know, we'll take care of this.

@mp911de mp911de self-assigned this Mar 30, 2021
@mp911de mp911de changed the title MappingMongoConverter should not use String.replaceAll for mapKeyDotReplacement Use StringUtils.replace(…) instead of String.replaceAll(…) for mapKeyDotReplacement Mar 30, 2021
@mp911de mp911de added this to the 3.1.7 (2020.0.7) milestone Mar 30, 2021
@mp911de mp911de removed the status: ideal-for-contribution An issue that a contributor can help us with label Mar 30, 2021
mp911de added a commit that referenced this issue Mar 30, 2021
…DotReplacement.

We now use StringUtils.replace(…) to replace the map key dot in MappingMongoConverter. StringUtils perform a plain search instead of using Regex which improves the overall performance.

Closes #3613
@HermanBovens
Copy link

HermanBovens commented Apr 13, 2022

@mp911de, note that this is a breaking change that, AFAICS, is not mentioned in any upgrade guide. We were using "\\+" in order to replace the dot with + (because + needs to be escaped in regex) and that code, although it still compiled, was broken.

It is only mentioned as a "new feature" in the release notes for this patch release:
https://github.com/spring-projects/spring-data-mongodb/releases/tag/3.1.7

In my opinion it shouldn't even have been included in a patch release since patch releases should not require any work to upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants