On Tue, 16 Jul 2024 18:17:00 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> David M. Lloyd has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review feedback
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 491:
>
>> 489: return 0;
>> 490: }
>> 491: }
>
> How is this different than
> `ObjectStreamClass.lookup(clazz).getSerialVersionUID()` ?
>
> And BTW, this misses the computation of the default SVUID and the SVUID of
> Records.
The `ObjectStreamClass` computes a default value if there is no field, so
serialization frameworks do not have any way to know whether a default value or
explicitly specified value is in use. That said - I'm looking over the custom
serializer code that we have and I think we switched to using
`ObjectStreamClass` a while ago so I can just remove this whole method.
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 505:
>
>> 503: );
>> 504:
>> 505: private static boolean isValidSerializable(Class<?> cl) {
>
> "Valid" is too broad to clearly identify the purpose.
> You might flip it to return true for all classes that use the default
> serialized object format.
>
> You might find that using the pattern language feature and "instanceof" would
> be effective and not need the small set.
Sure, I can rename this and invert it.
I haven't used the pattern feature much so far, but after reading JEP 441 again
and looking for other examples, while I see how an object can be matched with
`instanceof`, I can't figure out a way to use it to switch on an actual `Class`
object in this way. Am I missing something?
> test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 379:
>
>> 377: @SuppressWarnings("removal")
>> 378: ObjectOutputStream oos =
>> AccessController.doPrivileged((PrivilegedExceptionAction<ObjectOutputStream>)
>> () -> new ObjectOutputStream() {
>> 379: protected void writeObjectOverride(final Object obj) throws
>> IOException {
>
> The alternate invocation of the replacement defaultWriteObject should be
> hooked in here, to see how the whole mechanism works smoothly. Especially for
> nested serializations.
So far I'm unit testing each method to show that it works to its own
specification; in order to fully integrate with multiple classes and nested
objects, it seems as though I'd have to implement an entire custom
serialization framework for the test, which does not seem feasible; the
generated method does not relate in any way to nested serializations (that
would be solely the job of the framework which uses it).
The only difference I can see between directly using the generated
`default(Read|Write)Object` and one which uses them indirectly by way of a
`Ser#(read|write)Object()` calling through
`Object*Stream.default(Read|Write)Object()` is the call path through the test
classes and mocks. But, I could possibly add a couple more tests with another
pair of slightly different mocked streams which "tracks" the "current" object
(of which there would just be one) and makes sure that
`default(Read|Write)Object` also calls through the handle, just to show that it
works, though I don't think this adds any surety or even code coverage to the
overall tests since all of the additional code would exist only in the unit
test (i.e. I'd be writing a test that proves that the test works, not that the
feature works).
I do believe that this feature is fully covered by the existing tests, but if I
am missing some subtle detail we can discuss further of course.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769274124
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769278913
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769300945
