xmolecules / jmolecules-integrations

Technology integration for jMolecules
Apache License 2.0
79 stars 21 forks source link

Fix instantiation of records with multiple parameters #220

Closed barteksc closed 6 months ago

barteksc commented 7 months ago

This PR fixes RecordInstantiator, which is calling a record constructor with incorrectly ordered parameters.

odrotbohm commented 7 months ago

Would you mind elaborating a bit on what you think is broken with the current implementation? It feels a bit weird to change the existing tests in combination with the actual implementation, because it means that we can't verify whether the proposed change will still work for the previous test case. Does it make sense to add a new test case instead of changing the current one?

barteksc commented 6 months ago

I think it's broken, because sorting record parameters alphabetically and mapping parameters array from Hibernate doesn't mean that parameters will be correctly sorted to pass them to a record constructor.

It's difficult for me to explain it, please modify the current test case on the main branch by adding an age field:

    @Test // #98, #125
    void instantiatesRecord() {

        RecordInstantiator instantiator = new RecordInstantiator(Person.class);

        Object[] values = RecordInstantiator.IS_AFFECTED_HIBERNATE_VERSION
                ? new Object[] { "Matthews", 22, "Dave" }
                : new Object[] { 22, "Dave", "Matthews" };

        ValueAccess access = mock(ValueAccess.class);
        doReturn(values).when(access).getValues();

        assertThat(instantiator.instantiate(access, null))
                .isEqualTo(new Person("Matthews", 22, "Dave"));
    }

    record Person(String lastname, int age, String firstname) {}

The test will fail, as parameters won't be sorted correctly to instantiate a Person record. If you use a RecordInstantiator from my PR, the test will pass.

Sure, I will add my test case as a new one, next to the original test case.

odrotbohm commented 6 months ago

That's been very helpful, thanks. I now understand the issue with the original algorithm. I have a simplified version of your corrected algorithm locally here, which I am going to commit against this ticket.

barteksc commented 6 months ago

I'm happy you found a better way to fix it. Thanks!