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: issue with multiple aliases against same table #1761

Merged
merged 1 commit into from Dec 12, 2022

Conversation

ryaneorth
Copy link
Contributor

With the current implementation of TableExpression#assignAlias you cannot have multiple aliases against the same table. The code block below prevents the second reference from being added to the table alias list.

        if (this.baseExpression.isQueryKeyExpression()){
            QueryKeyExpression qkExpression = ((QueryKeyExpression)this.baseExpression);
            if (qkExpression.getTableAliases() != null && qkExpression.getTableAliases().keyAtValue(table) != null ) {
                return;
            }
        }

Removing the override of assignAlias in TableExpression and deferring to DataExpression's implementation results in the expected behavior.

@ryaneorth ryaneorth force-pushed the fix-duplicate-alias branch 4 times, most recently from 0fb74b5 to 5669628 Compare December 6, 2022 23:11
@rfelcman
Copy link
Contributor

rfelcman commented Dec 7, 2022

Sorry but during first brief check this PR can't pass due a following reasons:

  • It breaks current functionality please check details for EclipseLink PR build / Test on JDK 17 (pull_request) and continuous-integration/jenkins/pr-merge checks.
  • For functional fixes/changes like this is good to create some unit test (it's some kind of bug description and avoids regression in the future)

@rfelcman
Copy link
Contributor

rfelcman commented Dec 8, 2022

About run tests individually it's described at https://github.com/eclipse-ee4j/eclipselink/blob/master/jpa/eclipselink.jpa.testapps/README.md to pass to the IDE command like mvn test -pl :org.eclipse.persistence.jpa.testapps.jpql -Dtest=JUnitJPQLDateTimeTest -Pmysql like in IDEA IteliJ run profiles.
In this PR its about org.eclipse.persistence.core.test module not jpa.
Example command is
mvn test -pl :org.eclipse.persistence.core.test -Pmysql -Dtest=MapCollectionsTestModel -Ddb.platform=org.eclipse.persistence.platform.database.MySQLPlatform -Ddb.driver=com.mysql.cj.jdbc.Driver -Ddb.url=jdbc:mysql://localhost:3306/ecltests?allowPublicKeyRetrieval=true -Ddb.user=root -Ddb.pwd=root
but DB property values depends on Your local environment.

@ryaneorth
Copy link
Contributor Author

ryaneorth commented Dec 8, 2022

Thank you for your guidance and patience with me on this issue, @rfelcman ! I have implemented a solution that solves the original problem while not leading to regressions. I have also added a test case for the scenario, which fails without the associated change to TableExpression.

Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

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

LGTM

@rfelcman rfelcman merged commit 3aaf5f4 into eclipse-ee4j:master Dec 12, 2022
@ryaneorth
Copy link
Contributor Author

Thank you @rfelcman ! Out of curiosity, when will the next patch release be for 4.x.x?

@rfelcman
Copy link
Contributor

rfelcman commented Dec 12, 2022

Plan is end of Januray 2023
https://projects.eclipse.org/projects/ee4j.eclipselink/releases/4.0.1
Maven Snapshot based on Nightly build
https://ci.eclipse.org/eclipselink/job/eclipselink-nightly-master/
at
https://jakarta.oss.sonatype.org/content/repositories/snapshots/org/eclipse/persistence/eclipselink/4.0.1-SNAPSHOT/
after 1 hour if all tests will pass

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.

None yet

2 participants