-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Ensure correct ordering of delegates regardless of user input #3956
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,7 @@ | |
|
||
package org.springframework.kafka.support.serializer; | ||
|
||
import java.util.LinkedHashMap; | ||
import java.util.Map; | ||
import java.util.*; | ||
import java.util.Map.Entry; | ||
|
||
import org.apache.kafka.common.errors.SerializationException; | ||
|
@@ -32,13 +31,27 @@ | |
* @author Gary Russell | ||
* @author Artem Bilan | ||
* @author Wang Zhiyang | ||
* @author Mahesh Aravind V | ||
* | ||
* @since 2.7.9 | ||
* | ||
*/ | ||
public class DelegatingByTypeSerializer implements Serializer<Object> { | ||
|
||
private final Map<Class<?>, Serializer<?>> delegates = new LinkedHashMap<>(); | ||
private static final Comparator<Class<?>> DELEGATES_ASSIGNABILITY_COMPARATOR = | ||
(type1, type2) -> { | ||
|
||
if (type1.isAssignableFrom(type2)) { | ||
return 1; | ||
} | ||
if (type2.isAssignableFrom(type1)) { | ||
return -1; | ||
} | ||
|
||
return 0; | ||
}; | ||
|
||
private final Map<Class<?>, Serializer<?>> delegates = new TreeMap<>(DELEGATES_ASSIGNABILITY_COMPARATOR); | ||
|
||
private final boolean assignable; | ||
|
||
|
@@ -51,17 +64,23 @@ public DelegatingByTypeSerializer(Map<Class<?>, Serializer<?>> delegates) { | |
} | ||
|
||
/** | ||
* Construct an instance with the map of delegates; keys matched exactly or if the | ||
* target object is assignable to the key, depending on the assignable argument. | ||
* If assignable, entries are checked in the natural entry order so an ordered map | ||
* such as a {@link LinkedHashMap} is recommended. | ||
* @param delegates the delegates. | ||
* @param assignable whether the target is assignable to the key. | ||
* Construct an instance with the map of delegates. | ||
* If {@code assignable} is {@code false}, only exact key matches are considered. | ||
* If {@code assignable} is {@code true}, a delegate is selected if its key class | ||
* is assignable from the target object's class. When multiple matches are possible, | ||
* the most specific matching class is selected — that is, the closest match in the | ||
* class hierarchy. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No blank lines in method JavaDocs, please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have not been addressed yet |
||
* @param delegates the delegates | ||
* @param assignable true if {@link #findDelegate(Object, Map)} should consider assignability to | ||
* the key rather than an exact match. | ||
* | ||
* @since 2.8.3 | ||
*/ | ||
public DelegatingByTypeSerializer(Map<Class<?>, Serializer<?>> delegates, boolean assignable) { | ||
Assert.notNull(delegates, "'delegates' cannot be null"); | ||
Assert.noNullElements(delegates.values(), "Serializers in delegates map cannot be null"); | ||
|
||
this.delegates.putAll(delegates); | ||
this.assignable = assignable; | ||
} | ||
|
@@ -101,7 +120,7 @@ public byte[] serialize(String topic, Headers headers, Object data) { | |
return delegate.serialize(topic, headers, data); | ||
} | ||
|
||
private <T> Serializer<T> findDelegate(T data) { | ||
protected <T> Serializer<T> findDelegate(T data) { | ||
return findDelegate(data, this.delegates); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/* | ||
* Copyright 2021-2025 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.kafka.support.serializer; | ||
|
||
import java.util.LinkedHashMap; | ||
import java.util.Map; | ||
|
||
import org.apache.kafka.common.serialization.Serializer; | ||
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
|
||
/** | ||
* @author Mahesh Aravind V | ||
* | ||
*/ | ||
class DelegatingByTypeSerializerTests { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you create a new test class if we have one on the matter already: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is true, but I see tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MaheshAravindV It's better to have all the tests in |
||
@Nested | ||
class AssignableTest { | ||
@Test | ||
void shouldOrderDelegatesSoChildComesBeforeParent() { | ||
class Parent { } | ||
|
||
class Child extends Parent { } | ||
|
||
Serializer mockParentSerializer = mock(Serializer.class); | ||
Serializer mockChildSerializer = mock(Serializer.class); | ||
|
||
// Using LinkedHashMap to ensure the order is always wrong | ||
Map<Class<?>, Serializer<?>> delegates = new LinkedHashMap<>(); | ||
delegates.put(Parent.class, mockParentSerializer); | ||
delegates.put(Child.class, mockChildSerializer); | ||
|
||
DelegatingByTypeSerializer serializer = new DelegatingByTypeSerializer(delegates, true); | ||
|
||
|
||
Serializer childSerializer = serializer.findDelegate(mock(Child.class)); | ||
Serializer parentSerializer = serializer.findDelegate(mock(Parent.class)); | ||
|
||
|
||
assertThat(childSerializer).isEqualTo(mockChildSerializer); | ||
assertThat(parentSerializer).isEqualTo(mockParentSerializer); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if more robust algorithm would be like this: