Sitelet https://github.com/jsonapi-serializer/jsonapi-serializer/pull/223
Skip to content

Check has_one for instance variables as well - #223

Open
fiestacasey wants to merge 1 commit into
jsonapi-serializer:masterfrom
fiestacasey:master
Open

Check has_one for instance variables as well#223
fiestacasey wants to merge 1 commit into
jsonapi-serializer:masterfrom
fiestacasey:master

Conversation

@fiestacasey

@fiestacasey fiestacasey commented Jul 5, 2022 •
edited
Loading

Copy link
Copy Markdown

What is the current behavior?

We have a model that relies on has_one with through and an attribute accessor to find that parent object, something like this:

class User
  has_one :user_credit
  has_many :store_credits, through: :user_credit
end

class UserCredit
  belongs_to :user 
end

class StoreCredit
  belongs_to :user_credit
  has_one :user_credit, through: :user 

  validates_presence_of :user_credit_id

  attr_accessor :user_id

  before_validation :lookup_user_credit, if: -> { user_id.present? }

  def lookup_user_credit
    self.user_credit_id = UserCredit.find_or_create_by!(user_id: user_id).id
  end
end

and this extension breaks the lookup_user_credit because it is dynamically generating the reader function and only checking attributes and not instance variables. I dug through commit history in both this and the previous repo and I can't find the original reason this extension was added but I'm assuming its still needed?

What is the new behavior?

The change dynamically checks instance variables in the same fashion it is already checking attributes.

I did not add a unit test because the current extension is not hit and I'm hoping that this is just straightforward and basic enough to not have to add an entire suite of fixtures and/or rely on real active record models within in spec here but let me know.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@stas

stas commented Jul 27, 2022

Copy link
Copy Markdown
Collaborator

and this extension breaks the lookup_user_credit because it is dynamically generating the reader function and only checking attributes and not instance variables. I dug through commit history in both this and the previous repo and I can't find the original reason this extension was added but I'm assuming its still needed?

This is interesting. @fiestacasey does it work if you remove it completely and directly specify what's the name of the attribute via object_method_name or id_method_name?

I always wanted to get rid of these extensions, just ot make sure we don't mess up with user implementations.

N.B.: Apologies for the delay from my side.

@fiestacasey

fiestacasey commented Aug 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

@stas yes that would fix it but we're working with a pretty large monolith and refactoring it out would be a whole hassle.

Alternatively removing the extension entirely also resolves the problem and that is what we've done in the meantime on another fork:
master...wantable:jsonapi-serializer:master

We would be happy to create a PR for that solution if preferred but it could be a breaking change for someone out there, this solution felt like like a good middle ground.

@stas

stas commented Aug 1, 2022

Copy link
Copy Markdown
Collaborator

We would be happy to create a PR for that solution if preferred but it could be a breaking change for someone out there, this solution felt like like a good middle ground.

@fiestacasey I'm ok making the railtie part optional (maybe via an environmental variable) and adding a deprecation warning with the full removal in v2.5.0.

Appreciate if you could prepare a PR 🙇 🙏

@stas
stas self-requested a review January 31, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stas stas Awaiting requested review from stas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fiestacasey @stas