You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fury.apache.org by "OmCheeLin (via GitHub)" <gi...@apache.org> on 2025/05/31 10:49:55 UTC
[PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
OmCheeLin opened a new pull request, #2271:
URL: https://github.com/apache/fury/pull/2271
## What does this PR do?
raise an exception with error message when when registering serializer for Map/List.
## Related issues
- #2251
- #2270
## Does this PR introduce any user-facing change?
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
## Benchmark
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "OmCheeLin (via GitHub)" <gi...@apache.org>.
OmCheeLin commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2927323063
@chaokunyang I have modified the code as per your request.
But there is a question here:
failures in `ObjectStreamSerializerTest.java` and `ReplaceResolveSerializerTest.java`
details:
https://github.com/apache/fury/actions/runs/15362958237/job/43232318707?pr=2271
`java.util.concurrent.ConcurrentHashMap`, `ArrayList`, `com.google.common.collect.RegularImmutableList` etc. are all subclass of `Collection` and `Map`, but their Serializers are not extend AbstractCollectionSerializer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118614083
##########
java/fory-core/src/main/java/org/apache/fory/Fory.java:
##########
@@ -222,8 +226,40 @@ public void register(Class<?> cls, String namespace, String typeName) {
}
}
+ private void validateSerializer(
+ Class<?> type,
+ Object serializerOrClass,
Review Comment:
Could we make this type always be Class? This make the inferface more clean
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fory]
Posted by "OmCheeLin (via GitHub)" <gi...@apache.org>.
OmCheeLin closed pull request #2271: fix(java): raise exception when registering invalid serializer for Map/List
URL: https://github.com/apache/fory/pull/2271
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118615132
##########
java/fory-core/src/main/java/org/apache/fory/Fory.java:
##########
@@ -222,8 +226,40 @@ public void register(Class<?> cls, String namespace, String typeName) {
}
}
+ private void validateSerializer(
+ Class<?> type,
+ Object serializerOrClass,
+ Class<?> parentType,
+ Class<?> requiredSerializerBase) {
+ if (!parentType.isAssignableFrom(type)) {
+ return;
+ }
+ boolean valid = false;
+ if (serializerOrClass instanceof Class) {
+ valid = requiredSerializerBase.isAssignableFrom((Class<?>) serializerOrClass);
+ } else if (serializerOrClass instanceof Serializer) {
+ valid = requiredSerializerBase.isInstance(serializerOrClass);
+ }
+ if (!valid) {
+ throw new IllegalArgumentException(
+ "Serializer for type "
+ + type.getName()
+ + " must extend "
+ + requiredSerializerBase.getSimpleName()
+ + ", but got "
+ + serializerOrClass.getClass().getName());
+ }
+ }
+
+ private void validateCollectionSerializer(Class<?> type, Object serializerOrClass) {
+ validateSerializer(
+ type, serializerOrClass, Collection.class, AbstractCollectionSerializer.class);
Review Comment:
type may not be subclass of Collection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118614308
##########
java/fory-core/src/main/java/org/apache/fory/Fory.java:
##########
@@ -233,6 +269,7 @@ public <T> void registerSerializer(Class<T> type, Class<? extends Serializer> se
@Override
public void registerSerializer(Class<?> type, Serializer<?> serializer) {
+ validateCollectionSerializer(type, serializer);
Review Comment:
```suggestion
validateCollectionSerializer(type, serializer.getClass());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "OmCheeLin (via GitHub)" <gi...@apache.org>.
OmCheeLin commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118643838
##########
.gitignore:
##########
@@ -25,6 +25,7 @@ java/**/test-output
**/*.html
**/target
.idea/
+.vscode/
Review Comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118613288
##########
java/fory-core/src/main/java/org/apache/fory/Fory.java:
##########
@@ -242,6 +279,8 @@ public void registerSerializer(Class<?> type, Serializer<?> serializer) {
@Override
public void registerSerializer(Class<?> type, Function<Fory, Serializer<?>> serializerCreator) {
+ Serializer<?> serializer = serializerCreator.apply(this);
+ validateCollectionSerializer(type, serializer);
if (language == Language.JAVA) {
classResolver.registerSerializer(type, serializerCreator.apply(this));
Review Comment:
Please validate serializer in ClassResolver, users may use ClassResolver#registerSerializer directly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118613138
##########
.gitignore:
##########
@@ -25,6 +25,7 @@ java/**/test-output
**/*.html
**/target
.idea/
+.vscode/
Review Comment:
please don't ignore this, launch.json is used for debug bazel cpp for fury
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2926350721
Could you restore all files in .vscode and .idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2927367669
Serializer for ConcurrentHashMap
is [ConcurrentHashMapSerializer](https://github.com/apache/fury/blob/0d47fb847da558a444f8a35ab1073e80c9f3ad7b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/MapSerializers.java#L247), which is extends from AbstractMapSerializer . And serializers for ArrayList and RegularImmutableList are both extended from AbstractCollectionSerializer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2934126934
@OmCheeLin Please don't remove .idea directory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "OmCheeLin (via GitHub)" <gi...@apache.org>.
OmCheeLin commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2924948271
Should we exclude class under `java.util`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2271:
URL: https://github.com/apache/fury/pull/2271#discussion_r2118613318
##########
java/fory-core/src/test/java/org/apache/fory/ForyTest.java:
##########
@@ -554,6 +561,163 @@ public void testRegisterPrivateSerializer() {
serDeCheckSerializer(fory, obj, "Codec");
}
+ static final class InvalidList extends AbstractList<Object> {
+ @Override
+ public Object get(int index) {
+ throw new IndexOutOfBoundsException();
+ }
+
+ @Override
+ public int size() {
+ return 0;
+ }
+
+ public static final class InvalidListSerializer extends Serializer<InvalidList> {
+ public InvalidListSerializer(Fory fory) {
+ super(fory, InvalidList.class);
+ }
+
+ @Override
+ public void write(MemoryBuffer buffer, InvalidList value) {
+ // no-op
+ }
+
+ @Override
+ public InvalidList read(MemoryBuffer buffer) {
+ return new InvalidList();
+ }
+ }
+ }
+
+ static final class ValidList extends AbstractList<Object> {
+ @Override
+ public Object get(int index) {
+ throw new IndexOutOfBoundsException();
+ }
+
+ @Override
+ public int size() {
+ return 0;
+ }
+
+ public static final class ValidListSerializer extends AbstractCollectionSerializer<ValidList> {
+ public ValidListSerializer(Fory fory) {
+ super(fory, ValidList.class);
+ }
+
+ @Override
+ public Collection<?> onCollectionWrite(MemoryBuffer buffer, ValidList value) {
+ return Collections.emptyList();
+ }
+
+ @Override
+ public ValidList read(MemoryBuffer buffer) {
+ return onCollectionRead(Collections.emptyList());
+ }
+
+ @Override
+ public ValidList onCollectionRead(Collection collection) {
+ return new ValidList();
+ }
+ }
+ }
+
+ static final class InvalidMap extends AbstractMap<Object, Object> {
+ @Override
+ public Set<Entry<Object, Object>> entrySet() {
+ return Collections.emptySet();
+ }
+
+ public static final class InvalidMapSerializer extends Serializer<InvalidMap> {
+ public InvalidMapSerializer(Fory fory) {
+ super(fory, InvalidMap.class);
+ }
+
+ @Override
+ public void write(MemoryBuffer buffer, InvalidMap value) {
+ // no-op
+ }
+
+ @Override
+ public InvalidMap read(MemoryBuffer buffer) {
+ return new InvalidMap();
+ }
+ }
+ }
+
+ static final class ValidMap extends AbstractMap<Object, Object> {
+ @Override
+ public Set<Entry<Object, Object>> entrySet() {
+ return Collections.emptySet();
+ }
+
+ public static final class ValidMapSerializer extends AbstractMapSerializer<ValidMap> {
+ public ValidMapSerializer(Fory fory) {
+ super(fory, ValidMap.class);
+ }
+
+ @Override
+ public Map<?, ?> onMapWrite(MemoryBuffer buffer, ValidMap value) {
+ return Collections.emptyMap();
+ }
+
+ @Override
+ public ValidMap onMapCopy(Map map) {
+ return new ValidMap();
+ }
+
+ @Override
+ public ValidMap read(MemoryBuffer buffer) {
+ return onMapRead(Collections.emptyMap());
+ }
+
+ @Override
+ public ValidMap onMapRead(Map map) {
+ return new ValidMap();
+ }
+ }
+ }
+
+ @Test
+ public void testListAndMapSerializerRegistration() {
Review Comment:
Please add tests in ClassResolverTest.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] fix(java): raise exception when registering invalid serializer for Map/List [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2271:
URL: https://github.com/apache/fury/pull/2271#issuecomment-2927371613
You can take a look at failed tests, it's because they registered a special serializer for those classes:
```java
@Test(dataProvider = "foryCopyConfig")
public void testJDKCompatibleListCopy(Fory fory) {
fory.registerSerializer(ArrayList.class, new ObjectStreamSerializer(fory, ArrayList.class));
List<String> list = new ArrayList<>(ImmutableList.of("a", "b", "c", "d"));
copyCheck(fory, list);
fory.registerSerializer(LinkedList.class, new ObjectStreamSerializer(fory, LinkedList.class));
copyCheck(fory, new LinkedList<>(list));
fory.registerSerializer(Vector.class, new ObjectStreamSerializer(fory, Vector.class));
copyCheck(fory, new Vector<>(list));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org