Skip to content
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

Fix ReflectionCache to be serializable #634

Merged
merged 4 commits into from Feb 26, 2023
Merged

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Feb 22, 2023

Maybe fixes #295

I honestly don't understand the required specifications, but is this correct?
The following test confirms that the LRUMap set to reflectionCacheSize is retained, but not its contents.

import com.fasterxml.jackson.module.kotlin.ReflectionCache
import org.junit.Test
import java.io.FileInputStream
import java.io.FileNotFoundException
import java.io.FileOutputStream
import java.io.IOException
import java.io.ObjectInputStream
import java.io.ObjectOutputStream

class ReflectionCacheTest {
    private val serialFile = "./serial.ser"

    private fun mkSerialFile(obj: Any) {
        try {
            FileOutputStream(serialFile).use { fileOutStream ->
                ObjectOutputStream(fileOutStream).use { outStream ->
                    outStream.writeObject(obj)
                    outStream.flush()
                    outStream.reset()
                }
            }
        } catch (e: FileNotFoundException) {
            e.printStackTrace()
        } catch (e: IOException) {
            e.printStackTrace()
        }
    }

    private fun readSerialFile(): ReflectionCache {
        var reflectionCache: ReflectionCache? = null
        try {
            FileInputStream(serialFile).use { fileInStream ->
                ObjectInputStream(fileInStream).use { inStream ->
                    reflectionCache = inStream.readObject() as ReflectionCache
                }
            }
        } catch (e: ClassNotFoundException) {
            e.printStackTrace()
        } catch (e: IOException) {
            e.printStackTrace()
        } catch (e: Exception) {
            e.printStackTrace()
        }
        return reflectionCache ?: throw RuntimeException()
    }

    @Test
    fun test() {
        val cache = ReflectionCache(100)
        cache.kotlinFromJava(this::class.java.methods.first())
        mkSerialFile(cache)
        val tmp = readSerialFile()

        println()
    }
}

@cowtowncoder
Copy link
Member

@k163377 There are existing tests that can help. json-core f.ex has:

https://github.com/FasterXML/jackson-core/blob/2.15/src/test/java/com/fasterxml/jackson/core/TestJDKSerializability.java

so you need not write a file, but just write as byte[]/read from byte[]. Helper methods like:

    protected byte[] jdkSerialize(Object o) throws IOException
    {
        ByteArrayOutputStream bytes = new ByteArrayOutputStream(1000);
        ObjectOutputStream obOut = new ObjectOutputStream(bytes);
        obOut.writeObject(o);
        obOut.close();
        return bytes.toByteArray();
    }

    protected <T> T jdkDeserialize(byte[] raw) throws IOException
    {
        ObjectInputStream objIn = new ObjectInputStream(new ByteArrayInputStream(raw));
        try {
            return (T) objIn.readObject();
        } catch (ClassNotFoundException e) {
            fail("Missing class: "+e.getMessage());
            return null;
        } finally {
            objIn.close();
        }
    }

and with that I think 2 things need to be tested, if not already:

  1. ObjectMapper with Kotlin module registered (so that everything needed is java.io.Serializabe and
  2. ReflectionCache itself

Of these (1) might fail, and figuring out which things it refers to are not serializable can be some work. So here, just adding test for (2) would make sense, I think.

... although there is also difference between empty cache with no entries (not referencing things that might not be Serializable) and cache with entries. This is why test (1) is often done only after serializing once, deserializing once, to let referenced components be initialized.

But on verifying that a class is JDK serializable/deserializable, all that is needed is for write-then-read to succeed without exception.

I hope this helps.

@cowtowncoder
Copy link
Member

Oh, also; when testing a non-empty cache, I am pretty sure you will find out that actual cache LRUMaps need to be changed to transient (not serialized); and that readResolve() is needed to construct an empty cache instance. Otherwise LRUMap members were left as nulls as they are not (and should not be) serialized.

@k163377
Copy link
Contributor Author

k163377 commented Feb 25, 2023

@cowtowncoder
Thank you, that was very helpful.
The test I added has been pushed.

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

LGTM

@k163377 k163377 merged commit 7493f7d into FasterXML:2.15 Feb 26, 2023
@k163377 k163377 deleted the fix-#295 branch February 26, 2023 07:57
k163377 added a commit to k163377/jackson-module-kotlin that referenced this pull request Mar 3, 2023
cowtowncoder pushed a commit that referenced this pull request Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReflectionCache not Serializable
2 participants