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 instantiation of records with multiple parameters #220

Closed
wants to merge 2 commits into from

Conversation

barteksc
Copy link

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

Add a bit more complicated test case
@odrotbohm
Copy link
Member

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?

@odrotbohm odrotbohm self-assigned this Jan 30, 2024
@odrotbohm odrotbohm added the lifecycle:waiting-for-feedback Feedback from the original reporter(s) required label Jan 30, 2024
@barteksc
Copy link
Author

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
Copy link
Member

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.

odrotbohm added a commit that referenced this pull request Feb 24, 2024
We're now properly calculating the index of the values to look up based on the component order of the record constructor.
@odrotbohm odrotbohm added this to the 0.20 milestone Feb 24, 2024
@odrotbohm odrotbohm added module: jpa JPA runtime integration type: bug Bug and removed lifecycle:waiting-for-feedback Feedback from the original reporter(s) required labels Feb 24, 2024
@odrotbohm odrotbohm closed this Feb 24, 2024
@barteksc
Copy link
Author

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

odrotbohm added a commit that referenced this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: jpa JPA runtime integration type: bug Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants