Add support for custom primary key on related_models

[thibaudgg]

Discussed on #10 (comment)

(Edit by @valscion: Closes #13)

[valscion]

Thanks! Good work! 👍 😄 It's nice to get a PR :)

Weird. The tricky_operations_spec.rb seems to fail when it is run on its own with bundle exec rspec spec/requests/tricky_operations_spec.rb.

Also, now I remember why we used find. find_by_key returns only the first element of the relationship — so when we try to specify it for multiple resources, it doesn't call the authorizer with them all but only with the first one.

I created a PR where I implemented the tricky operations specs, #13. The diff is interesting, as it has the checks done for the authorizer contract instead of Pundit. We actually forgot to update the tricky operations specs to use that strategy and I'm sorry for that 😕

If you modify your PR to use authorization level stubs, it will fail, as this check in the #13 PR wouldn't be called with all the new comments, but only the first one:

before { allow_operation('replace_fields', article, new_comments) }

It might be an issue in JR gem itself — if there would be a way around it, it'd be great.

I'll check again on this PR tomorrow.

[thibaudgg]

Ok, I'll have a look again tomorrow as well.

[thibaudgg]

Hei @valscion, I rebased my PR on your tricky-specs branch and updated the related_models query to make all specs pass again. It should be good now.

[valscion]

I don't quite follow why you must specify an id here? Is it that this PR changes the Article resource to have its ID generated client-side like as an UUID? Auto-increment-like behavior no longer works, I assume.

[thibaudgg]

The reason is as we're forcing the presence of external_id in the db we need to set it from somewhere. In a production env it could be set from a DB pgsql function or something (UUID).

Auto-increment-like behavior no longer works, I assume.

You're correct, but I don't think it's a big issue for the specs right?

[valscion]

Yeah I'm fine with this 👍 It didn't come to my mind that it could be set on the database side, too. I agree it's not necessary to complicate the test database setup with ID generation, so it's ok that the ID is specified here.

Should you, though, use another id as 1 here? Doesn't the fixtures already specify an article with the similar ID? I wonder why this request didn't fail if there already was an article in the DB 😕

[valscion]

Super amazing work, thank you! 😍

Besides the one oddity with the ID in the POST spec, I have no issues whatsoever with this PR. Amazing to have you try this gem on your real, large API 👍

EDIT: And sorry for hitting the "Close and comment" button accidentally 😳

[thibaudgg]

@valscion thanks my pleasure, I'll update the ID value with something more specific.

I just get an issue with a polymorphic association, so maybe hold a little bit before merging. I'm not sure that it's directly link to that change or if polymorphism will need a separated PR.

[valscion]

I'm not sure that it's directly link to that change or if polymorphism will need a separated PR.

I have a feeling that we'll get issues with polymorphic associations elsewhere, too, which have been present even before this PR. But I'll wait :)

[thibaudgg]

Ok, I fixed my polymorphic issue by updating the related_models method with:

def resource_class_for_relationship(assoc_name)
  @operation.resource_klass.resource_for(assoc_name.to_s)
end

def related_models
  data = @operation.options[:data]
  return [] if data.nil?

  [:to_one, :to_many].flat_map do |rel_type|
    data[rel_type].flat_map do |assoc_name, assoc_value|
      case assoc_value
      when Hash # Polymorphic relationship
        resource_class = resource_class_for_relationship(assoc_value[:type])
        resource_class.find_by_key(assoc_value[:id], context: @operation.options[:context])._model
      else
        resource_class = resource_class_for_relationship(assoc_name)
        primary_key = resource_class._primary_key
        resource_class._model_class.where(primary_key => assoc_value)
      end
    end
  end
end

I think this can be done in this PR, do you already have something in mind for a polymorphic association in the dummy app? A Tag object for comments and articles?

[valscion]

I think this can be done in this PR, do you already have something in mind for a polymorphic association in the dummy app? A Tag object for comments and articles?

Feel free to do it here 👍 A Tag object seems reasonable.

[thibaudgg]

Polymorphic relationship support added, tested with all resources in our app, no more spec failures.

[valscion]

This is so good, thank you! Everything seems to be in order here.

[thibaudgg]

Awesome, thanks!