Skip to content

Commit

Permalink
GH-1858: Fix Regression in JsonDeserializer
Browse files Browse the repository at this point in the history 
Resolves #1858

Do not add mapping class packages if trusted packages is already all (`*`).

**cherry-pick to 2.7.x**
  • Loading branch information
@garyrussell @artembilan
garyrussell authored and artembilan committed Jul 7, 2021
1 parent 23f0404 commit acc7fcf
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 0 deletions.
3 changes: 3 additions & 0 deletions .../main/java/org/springframework/kafka/support/converter/DefaultJackson2JavaTypeMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public void setTypePrecedence(TypePrecedence typePrecedence) {
*/
@Override
public void addTrustedPackages(String... packagesToTrust) {
if (this.trustedPackages.size() == 0) {
return;
}
if (packagesToTrust != null) {
for (String trusted : packagesToTrust) {
if ("*".equals(trusted)) {
Expand Down
11 changes: 11 additions & 0 deletions ...ka/src/test/java/org/springframework/kafka/support/serializer/JsonSerializationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,17 @@ void testTrustMappingPackages() {
.contains(Foo.class.getPackageName() + ".*");
}

@SuppressWarnings("unchecked")
@Test
void testTrustMappingPackagesWithAll() {
JsonDeserializer<Object> deser = new JsonDeserializer<>();
Map<String, Object> props = Map.of(
JsonDeserializer.TRUSTED_PACKAGES, "*",
JsonDeserializer.TYPE_MAPPINGS, "foo:" + Foo.class.getName());
deser.configure(props, false);
assertThat(KafkaTestUtils.getPropertyValue(deser, "typeMapper.trustedPackages", Set.class)).isEmpty();
}

@SuppressWarnings("unchecked")
@Test
void testTrustMappingPackagesMapper() {
Expand Down

2 comments on commit acc7fcf

@idkw
Copy link

@idkw idkw commented on acc7fcf Jul 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garyrussell Wouldn't it be cleaner to have something like this ?
Because otherwise there is now a function called addTrustedPackages() which looks innocent but actually lies under the hood by not really adding in some cases


	private final Set<String> trustedPackages = new LinkedHashSet<>(TRUSTED_PACKAGES);
	private final boolean trustAnyPackage =false;

        (...)

        @Override
	public void addTrustedPackages(String... packagesToTrust) {
		if (packagesToTrust != null) {
			for (String trusted : packagesToTrust) {
				if ("*".equals(trusted)) { 
                                        this.trustAnyPackage = true;
				}
				else {
					this.trustedPackages.add(trusted);
				}
			}
		}
	}

        (...)

       private boolean isTrustedPackage(String requestedType) {
                if(this.trustAnyPackage) {
                        return true;
                }

                String packageName = ClassUtils.getPackageName(requestedType).replaceFirst("\\[L", "");
                for (String trustedPackage : this.trustedPackages) {
                    if (PatternMatchUtils.simpleMatch(trustedPackage, packageName)) {
	                return true;
                    }
                }
                return false;
	}

@garyrussell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's functionally the same, but feel free to submit a pull request if you feel strongly about it.

Please sign in to comment.