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

RETURNING clause mistakenly added to CREATE and ALTER table statements #2119

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

elpete
Copy link
Contributor

@elpete elpete commented Apr 5, 2021

When trying to create or alter a table that includes a foreign key constraint and an ON DELETE or ON UPDATE clause, the parser incorrectly marks the CommandType as DELETE or UPDATE, respectively. Combined with a returningColumns of {"*"} (which is the default when just passing in the autoGeneratedKeys flag to execute in PgStatement) this causes a RETURNING clause to be added where it is invalid.

This PR adds four test cases showing the issue.

This PR also adds two new CommandType values for CREATE and ALTER to detect this scenario and prevent the RETURNING clause from being added. This is my first PR to this project and while the tests are passing, there may be a better implementation. I defer to the more seasoned contributors.

I want to add that while using executeUpdate would solve this issue handily, it is unfortunately not exposed by the application framework. They simply use public boolean execute(String sql, int autoGeneratedKeys) and trust this JDBC driver to handle it. I am also working on that project to be more correct in their implementation as well. I think this feature will be a benefit to all regardless of any change in downstream frameworks.

Thank you for your time!

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does ./gradlew autostyleCheck checkstyleAll pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order? N/A

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain. N/A
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

@davecramer
Copy link
Member

I was going to ask how this was possible, since we don't normally see this, but reading the description the project uses execute, instead of executeQuery.

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.

2 participants