Skip to content

Fix quoting issue on generate sql statements#1395

Closed
MarcelEeken wants to merge 1 commit intoJSONAPI-Resources:masterfrom
MarcelEeken:fix-quoting-issue
Closed

Fix quoting issue on generate sql statements#1395
MarcelEeken wants to merge 1 commit intoJSONAPI-Resources:masterfrom
MarcelEeken:fix-quoting-issue

Conversation

@MarcelEeken
Copy link
Contributor

Using the previous method to generate the quote string could lead to incorrect sql statement where double quotes were used for select statements in MySql.

By using the quote_table_name function from Rails, the correct quoting mechnism will be automatically used for the specific database.

Fixes #1369

All Submissions:

  • I've checked to ensure there aren't other open Pull Requests for the same update/change.
  • I've submitted a ticket for my issue if one did not already exist.
  • My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
  • I've used Github auto-closing keywords in the commit message or the description.
  • I've added/updated tests for this change.

Bug fixes and Changes to Core Features:

  • I've included an explanation of what the changes do and why I'd like you to include them.
  • I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

  • Maintains compliance with JSON:API
  • Adequate test coverage exists to prevent regressions

Using the previous method to generate the quote string could lead to incorrect
sql statement where double quotes were used for select statements in MySql.

By using the `quote_table_name` function from Rails, the correct quoting
mechnism will be automatically used for the specific database.

Fixes JSONAPI-Resources#1369

def quote(field)
"\"#{field.to_s}\""
ActiveRecord::Base.connection.quote_table_name(field)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you write a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an existing open mr that added mysql to the CI workflow. That was opened to cover the same case as this fix.

Would that mr be enough of a test-case
https://github.com/cerebris/jsonapi-resources/pull/1370

@bf4
Copy link
Collaborator

bf4 commented Feb 2, 2023

@bf4 bf4 changed the base branch from master to release-0-10 February 2, 2023 21:33
@bf4 bf4 changed the base branch from release-0-10 to master February 2, 2023 21:33
@Kallin
Copy link

Kallin commented Oct 18, 2023

Here is an example of getting a test to fail for this issue:
Kallin@1743848

as others have said, adding MySQL to CI matrix would surface this issue.

@bf4 bf4 changed the base branch from master to release-0-10 November 1, 2023 14:22
@bf4 bf4 changed the base branch from release-0-10 to master November 1, 2023 14:23
@bf4
Copy link
Collaborator

bf4 commented Nov 1, 2023

resolved in v011-dev via https://github.com/cerebris/jsonapi-resources/pull/1400 and https://github.com/cerebris/jsonapi-resources/pull/1370 if this is needed in another branch, let's re-use those learnings and apply there

@bf4 bf4 closed this Nov 1, 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.

MySQL Syntax Error in ActiveRelationResource (0.10.x)

3 participants