feat(java): Invoke callback on read failure #1870
Conversation
yoohaemin
force-pushed
the
yoohaemin/callback-on-read-failure
branch
from
b1e75ba to
63ac97a
Compare
| // Field doesn't exist in current class, invoke field mismatch callback. | ||
| Expression fieldMismatchCallback = | ||
| Expression.Invoke.inlineInvoke( | ||
| new Expression.Reference(FURY_NAME, TypeRef.of(Fury.class)), | ||
| "getFieldMismatchCallback", | ||
| TypeRef.of(FieldMismatchCallback.class), | ||
| /* needTryCatch */ false); |
There was a problem hiding this comment.
This can be a private field, instead of being instantiated every time. I'm not used to this kind of metaprogramming so I need to figure out how to do this.
| } | ||
|
|
||
| public FuryBuilder withFieldMismatchCallback(FieldMismatchCallback callback) { | ||
| this.fieldMismatchCallback = callback; |
There was a problem hiding this comment.
Maybe it can be confusing that the callback can be overridden, not added on top of already registered ones?
| } | ||
| } | ||
|
|
||
| static Object readFieldValue( |
There was a problem hiding this comment.
This is a rather big method; I'm not sure if HotSpot can compile this. In any case, this shouldn't be in the hot path.
|
Is it possible to deserialize again by taking fields that don't exist in the class and unifying them in a map property? |
| if (buffer.readByte() == Fury.NULL_FLAG) { | ||
| return null; | ||
| } else if (fury.isBasicTypesRefIgnored()) { | ||
| return readFinalObjectFieldValue( |
There was a problem hiding this comment.
Seems this branch can be removed
| if (buffer.readByte() == Fury.NULL_FLAG) { | ||
| return null; | ||
| } else if (fury.isBasicTypesRefIgnored()) { | ||
| return readFinalObjectFieldValue( |
| if (buffer.readByte() == Fury.NULL_FLAG) { | ||
| return null; | ||
| } else if (fury.isBasicTypesRefIgnored()) { | ||
| return readFinalObjectFieldValue( |
| new Literal(descriptor.getTypeName()), | ||
| new Literal(descriptor.getName())); | ||
|
|
||
| Expression invokeHandler = |
There was a problem hiding this comment.
The onMismatch can be cached as a field using getOrCreateField function
| "forName", | ||
| TypeRef.of(Class.class), | ||
| true, | ||
| new Literal(beanClass.getName())), |
There was a problem hiding this comment.
Use Literal.ofXXX instead
| fieldMismatchCallback, | ||
| "onMismatch", | ||
| TypeRef.of(FieldMismatchCallback.FieldAdjustment.class), | ||
| new Expression.StaticInvoke( |
There was a problem hiding this comment.
Use org.apache.fury.builder.CodecBuilder#beanClassExpr(java.lang.Class<?>) instead
|
Hi @yoohaemin , is there anything I can help to put this PR forward? |
|
@chaokunyang hey, thanks for the ping. I haven't forgotten about this PR, but I am a bit swamped with other things personally and have no time to push it forward. If you want to push it forward yourself, please feel free to do so. Otherwise, I will work on it when I have time. Unfortunately, I can't tell you when exactly I will have time, but probably this month would be hard. |
|
@yoohaemin thanks for continuing working on this pr. This ia not urgent, you can take your time. I will take a look at the ci failures to see whether I can provide more inputs |
|
@chaokunyang Are merge commits preferred or is rebasing preferred? |
merging is prefer when there is a conflict |
## Why? <!-- Describe the purpose of this PR. --> ## What does this PR do? Support type converters for comaptible mode, so the deserialization peer can have different type for fields, and fory can convert to target type instead of ignoring it. ## Related issues Closes #2636 #1870 ## Does this PR introduce any user-facing change? <!-- If any user-facing interface changes, please [open an issue](https://github.com/apache/fory/issues/new/choose) describing the need to do so and update the document if necessary. Delete section if not applicable. --> - [ ] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? ## Benchmark <!-- When the PR has an impact on performance (if you don't know whether the PR will have an impact on performance, you can submit the PR first, and if it will have impact on performance, the code reviewer will explain it), be sure to attach a benchmark data here. Delete section if not applicable. -->
3 participants
What does this PR do?
See discussion: #1848 (comment)
When Fury encounters a value that does not exist in the current class definition, it can still read the value. This PR allows the user of the library to specify what to do with the value. The user can specify a function
Object -> Objectto update the value, and also specify aFieldfor that adjusted value to be set.Related issues
#1848
Does this PR introduce any user-facing change?
Benchmark
TODO