Skip to content

Works around primary key being cast due to pluck bug#1159

Merged
lgebhardt merged 2 commits intoJSONAPI-Resources:masterfrom
butchmarshall:pluck_fix
Jul 10, 2018
Merged

Works around primary key being cast due to pluck bug#1159
lgebhardt merged 2 commits intoJSONAPI-Resources:masterfrom
butchmarshall:pluck_fix

Conversation

@butchmarshall
Copy link
Copy Markdown
Contributor

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:

A unit test has been included with this pull request which reproduces the issue described.

Reviewer Checklist:

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

This fixes issue #1156 and provides a complete set of fixes for pull request #986.

Copy link
Copy Markdown
Contributor

@lgebhardt lgebhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Could you please rework the tests so this passes on rails 4.2?

@butchmarshall
Copy link
Copy Markdown
Contributor Author

Can do. It may take me a day or so but it'll definitely get done. Thanks!

@butchmarshall
Copy link
Copy Markdown
Contributor Author

butchmarshall commented Jul 10, 2018

Looks like it's failing because I added a key_type :uuid JSONAPI::Resource test (none had existed before).

Most likely related to rails/rails#24857

Workaround is to set self.primary_key = "id" on the resource.

I've pushed a fix (note my commenting out of the final assert_equal in my new test)

@lgebhardt lgebhardt merged commit 7c0c7ab into JSONAPI-Resources:master Jul 10, 2018
@lgebhardt
Copy link
Copy Markdown
Contributor

@butchmarshall Thanks for tracking that down and fixing!

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