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

has_many with different public field #29

Closed
jsmestad opened this issue Oct 14, 2014 · 16 comments
Closed

has_many with different public field #29

jsmestad opened this issue Oct 14, 2014 · 16 comments

Comments

@jsmestad
Copy link
Contributor

I'm having issues using the has_many association helper and want to know if I am doing some obviously incorrect before I explore putting together a PR.

Here is an example of the models. We use normal integer primary keys internally, but externally we only display the UUID.

class Domain < ActiveRecord::Base
  # id (pk) integer
  # guid
    has_many :dns_records, class_name: DNS::Record

end

class DNS::Record < ActiveRecord::Base
  # id (pk) integer
  # domain_id - integer
  # guid
  belongs_to :domain
end

This is what we are trying to do:

class DomainResource < JSONAPI::Resource
  # key :id
  attributes :id, :name, :tld
  attribute :created_at, format: :date_time
  attribute :updated_at, format: :date_time

  has_many :records, class_name: 'DNS::Record', key: 'dns_record_ids', primary_key: 'guid'

  def id
    @model.guid
  end

class RecordResource < JSONAPI::Resource
  attribute :id

  def id
    @model.guid
  end
end

However this is not working. Any tips? Is this a use-case that is unsupported and I should put together a PR?

@lgebhardt
Copy link
Member

Do you have a RecordResource declared? You will need that if you don't have it. If you do could you post that as well? You shouldn't need the class_name or key since those should be the defaults. I think if you have an id method on the RecordResource you would be able to get rid of the primary_key as well.

@jsmestad
Copy link
Contributor Author

@lgebhardt updated; I added that the model is actually namespaced under DNS.

@lgebhardt
Copy link
Member

I haven't tested the code with namespaced classes. Could you put together a small sample app that reproduces the problem? Then I'll look at what it will take to support that case.

@jsmestad
Copy link
Contributor Author

Sure can

@jsmestad
Copy link
Contributor Author

@lgebhardt alright its up on jsmestad/jsonapi-resources-test-failure.

If you run rspec you can see the error:

master ✔ $ rspec spec/acceptance/domains_spec.rb
F

Failures:

  1) Domains GET /domains Listing all domains
     Failure/Error: Unable to find matching line from backtrace
     NameError:
       undefined method `record_ids' for class `Domain'

@lgebhardt
Copy link
Member

@jsmestad Try:

class DomainResource < JSONAPI::Resource
  attributes :id, :name

  has_many :records, key: 'dns_record_ids'

  def id
    @model.guid
  end
end

class RecordResource < JSONAPI::Resource
  attributes :id, :name

  model_name 'Dns::Records'

  def id
    @model.guid
  end
end

The key needs to be the key that's used on the underlying model. Since the RecordResource isn't backed by a model named Record you need to tell it the name of the backing model with the model_name method. I'm glad to see that it works with the namespace despite the fact that I never thought to test it. Let me know it that works for you.

@jsmestad
Copy link
Contributor Author

@lgebhardt so that solves that error. Now for the follow up. I want to have links display the public guid and NOT the internal PK.

turning this:

{"domains"=>[{"id"=>"347b0611-8b60-411a-ac3e-e22117c47772", "name"=>"foobar", "links"=>{"records"=>[1, 2]}}]}

into this:

"domains"=>[{"id"=>"347b0611-8b60-411a-ac3e-e22117c47772", "name"=>"foobar", "links"=>{"records"=>["e3243950-bb05-4d0b-8d34-1b4821eca360", "0bbe4f4b-ffdb-4c87-8ede-2feebb46bb94"}}]}

@lgebhardt
Copy link
Member

Glad that worked. I won't be able to look the id issue until later tonight or first think in the morning.

@jsmestad
Copy link
Contributor Author

alright; thanks again for helping out. I've been sifting through the codebase to find where to patch it but I cannot even come up with where this gets called ;)

@jsmestad
Copy link
Contributor Author

Dug in a bit more. It appears the culprit line is resource_serializer.rb

It is relying on the ActiveRecord association's dns_record_ids method.


Temporary workaround pushed to the branch: workaround


My thought is the default for id should be :to_param. Which for most applications will be id but for those using different public identifiers, or even slugs, to_param would allow drop-in support. Thoughts?

@lgebhardt
Copy link
Member

You could rename your method to record_guids if you set the key on the has_many association.

class DomainResource < JSONAPI::Resource
  attributes :id, :name

  has_many :records, key: :record_guids

  def id
    @model.guid
  end

  def record_guids
    @model.dns_records.collect(&:guid)
  end
end

I'll need a bit of time to look at the implications of to_param, but at first glance it looks like the right way to go.

@lgebhardt
Copy link
Member

It looks like to_param could be made to work for the serialization. This would remove need for the key option. But without the key option we won't be able to process incoming requests that use an alternate key. We could rely on the underlying model's primary_key but I think that would break your setup too. At this point I'm thinking the workaround is probably your best bet.

@jsmestad
Copy link
Contributor Author

What about an embed_with: or even a serializer: option like ActiveModelSerializers?

@lgebhardt
Copy link
Member

I think you've exposed a flaw in the way JR handles keys. I'm going to experiment with a few things to see if I can improve the way JR deals with keys. We should be able to specify the primary key on the resource and use that whenever we represent the resource. If I get that working I still think you will need to implement a workaround, but it will be on the finders since those won't use the GUIDs.

@lgebhardt
Copy link
Member

@jsmestad I just released version 0.0.8, which reworks the key option in resources and associations. I also put a branch of your test-failure app up at https://github.com/lgebhardt/jsonapi-resources-test-failure/tree/jr_key_changes that shows how you can use the new version of JR to do what you are trying to do with UUIDs.

@lgebhardt
Copy link
Member

Closing this now since it should be fixed. Please reopen if it isn't fixed.

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

No branches or pull requests

2 participants