Skip to content

Fix JSONAPI::PathSegment::Relationship#eql?#1334

Merged
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
larskanis:fix-relationship-eql
Jan 8, 2021
Merged

Fix JSONAPI::PathSegment::Relationship#eql?#1334
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
larskanis:fix-relationship-eql

Conversation

@larskanis
Copy link
Copy Markdown
Contributor

@larskanis larskanis commented Nov 11, 2020

Relationship objects are used as key in @join_details in JoinManager . This hash contains two types of objects: String and Relationship. In case of hash collisions in @join_details the method Relationship#eql? is called. It currently fails when comparing with a string key.

Fixes #1333

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:

There's no reliable way to test for hash collisions. It would be possible to specifically test for Relationship#eql?("") . Shall I add such a test?

Reviewer Checklist:

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

@larskanis
Fix JSONAPI::PathSegment::Relationship#eql?
3e9f9f3 
Relationship objects are used as key in @join_details in JoinManager.
This hash contains two types of objects: String and Relationship
In case of hash collisions in @join_details the method Relationship#eql? is called.
It currently fails when comparing with a string key.

Fixes JSONAPI-Resources#1333
atsjj added a commit to atsjj/jsonapi-resources that referenced this pull request Dec 17, 2020
@atsjj
merging in from JSONAPI-Resources#1334
c421eca
@atsjj
Copy link
Copy Markdown

atsjj commented Dec 17, 2020

This bug has been biting me bad lately and it's really hard to reproduce. Going to merge your PR into my branch and ship it out.

@lgebhardt lgebhardt merged commit 3ed4d4f into JSONAPI-Resources:master Jan 8, 2021
@lgebhardt
Copy link
Copy Markdown
Contributor

lgebhardt commented Jan 8, 2021

@larskanis Thanks for finding and fixing that

@Vince-Smith
Copy link
Copy Markdown

Vince-Smith commented Jan 28, 2021

I ran into this today after upgrading from ruby 2.5.3 to 2.7.2. Looking at the fix I can't figure out how this would be related to the version upgrade, and never came across this before.

Does anyone know if this is a bug specific to 2.7? Or is it just really unlikely?

@atsjj
Copy link
Copy Markdown

atsjj commented Feb 1, 2021

I ran into this today after upgrading from ruby 2.5.3 to 2.7.2. Looking at the fix I can't figure out how this would be related to the version upgrade, and never came across this before.

Does anyone know if this is a bug specific to 2.7? Or is it just really unlikely?

In my experience chasing this particular bug, it's presented itself so sporadically that it was quite the chore chasing down. @larskanis was the only other person I found and he managed to create a failing test and his methodology was the only one that could prove it's existence.

@batter
Copy link
Copy Markdown

batter commented Sep 29, 2022

I looked through the source and it doesn't seem like this ever made it into an official release? Is this fix the same as #1368 ?

Confused why this is in the master branch, but the merge commit doesn't show up in the list of commits for v0.10.7

@mikhaillopandia
Copy link
Copy Markdown

mikhaillopandia commented Jun 7, 2023

Hi @lgebhardt ! Will this fix be included to the next release? We still facing the issue on 0.10.4 in production.

@lgebhardt
Copy link
Copy Markdown
Contributor

lgebhardt commented Jun 7, 2023

@mikhaillopandia Thanks for the reminder. I'll get this merged into the v0.10 branch and see if I can get a release out this week.

@larskanis larskanis deleted the fix-relationship-eql branch June 9, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sporadic error when fetching related objects

6 participants

@larskanis @atsjj @lgebhardt @Vince-Smith @batter @mikhaillopandia