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 for Postgres SQL generation error #1287

Merged
merged 2 commits into from
Oct 30, 2019
Merged

Fix for Postgres SQL generation error #1287

merged 2 commits into from
Oct 30, 2019

Conversation

lgebhardt
Copy link
Member

@lgebhardt lgebhardt commented Oct 8, 2019

This fixes an issue reported in Gitter where the generated SQL isn't compatible with Postgres.

ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list

This fix tracks the fields used for sorting and ensures they are in the SQL used to pluck the results. The sort fields are tracking in an option and can therefore be set in a sort's apply callable. The join_manager was also moved into the options in order to allow correct tracking of joins.

I still need to update the documentation for this. I'll try to update this PR with some examples soon.

Most of the code changes are related to the tests and getting Travis testing with postgres. As of now you need to manually create the database (rake won't work) and set the DATABASE_URL environment variable to test locally against postgres. For example:

DATABASE_URL=postgresql://postgres@localhost/jr_test bundle exec rake test

Eventually I'll see if I can load the default rails rake tasks so a db:create can work, but that will be another PR.

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.

New Feature Submissions:

  • I've submitted an issue that describes this feature, and received the go ahead from the maintainers.
  • My submission includes new tests.
  • My submission maintains compliance with JSON:API.

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

@lgebhardt lgebhardt changed the title Fix for Postgres SQL generation error WIP Fix for Postgres SQL generation error Oct 8, 2019
@lgebhardt lgebhardt changed the title WIP Fix for Postgres SQL generation error Fix for Postgres SQL generation error Oct 9, 2019
Copy link

@CharlieIGG CharlieIGG left a comment

Choose a reason for hiding this comment

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

LGTM. Can we get this merged 🙏 ?

@lgebhardt lgebhardt merged commit cc96793 into master Oct 30, 2019
@lgebhardt lgebhardt deleted the test_on_pg branch July 30, 2020 12:34
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