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

Block for conditional field is always called #49

Open
ezekg opened this issue Dec 20, 2016 · 11 comments
Open

Block for conditional field is always called #49

ezekg opened this issue Dec 20, 2016 · 11 comments

Comments

@ezekg
Copy link

ezekg commented Dec 20, 2016

Example

relationship :user, unless: -> { @object.user.nil? } do
  link :related do
    @url_helpers.v1_user_path @object.user
  end
end

Expected outcome

The relationship block is not called when the conditional fails.

Actual outcome

The relationship block is called regardless of the conditional failing; only the display of the serialized relationship is changed. This requires additional checks within the block, which makes the code less readable (especially for nested resource routes) and duplicates the condition:

relationship :user, unless: -> { @object.user.nil? } do
  link :related do
    @url_helpers.v1_user_path @object.user if @object.user.present?
  end
end

Solution

Do not call the block if the condition fails.


Would you be open to a pull request?

@beauby
Copy link
Member

beauby commented Dec 21, 2016

@ezekg You are right – at the moment links are built eagerly (only the related objects are constructed lazily). However, a simple fix for your use-case (that would be in-line with the spirit of the spec) would be to expose the related user via some nested route (like /posts/3/user), rather than the canonical resource url (/users/:id).

Regarding the former, I am currently refactoring that part to avoid eagerly building stuff, so it should be fixed for the next release.

@beauby
Copy link
Member

beauby commented Dec 22, 2016

@ezekg See json-api/json-api#1134 and related issues for an overview of why exposing the related link as /posts/3/user is the most flexible solution.

@ezekg
Copy link
Author

ezekg commented Dec 22, 2016

As much as I'd like to implement that, at the moment it brings in too much overhead into developing the API. I'd need to create new routes/controllers for each relationship, and write tests for them.

Appreciate the guidance though. I'm going to create a ticket as a reminder to come back to this.

@beauby
Copy link
Member

beauby commented Feb 17, 2017

@ezekg Would you mind writing a regression test for this?

@ezekg
Copy link
Author

ezekg commented Feb 17, 2017

Definitely. I'll try and do that this weekend.

@dylanlewis89
Copy link

Out of curiosity, does the library currently support specifying any conditional rendering of attributes in the serializer via the exposed instance variables?

For example:

class SerializableBar < JSONAPI::Serializable::Resource
  type 'bars'

  attribute :always_show_me
  attribute :conditionally_show_me, unless: -> { @authenticated }
end

This requested feature was the closest I could find to this and just wanted to see if I was missing anything obvious that already existed in the library.

@beauby
Copy link
Member

beauby commented Jul 24, 2017

@dylanlewis89 Yes it does, you just have to call extend JSONAPI::Serializable::Resource::KeyFormat in your serializer in order to enable it.

@beauby
Copy link
Member

beauby commented Jul 24, 2017

@dylanlewis89 Sorry, I obviously meant JSONAPI::Serializable::Resource::ConditionalFields rather than JSONAPI::Serializable::Resource::KeyFormat.

@dylanlewis89
Copy link

Thanks @beauby for the quick and helpful response!

For anyone tackling this issue in the future, the 0.2.1 release introduces the extend JSONAPI::Serializable::Resource::ConditionalFields syntax. Earlier versions use prepend instead of extend

@beauby
Copy link
Member

beauby commented Sep 12, 2017

@ezekg So if you're still interested in fixing this, it's just a matter of delaying the instance_evaling here until either as_jsonapi or related_resources are called.

@ezekg
Copy link
Author

ezekg commented Oct 4, 2017

Hey, I just updated my API to use the latest version of jsonapi-rb/rails so I can dig into fixing this now! I was trying to hold off until I could get around to updating, as I was still on v0.1.1. 🙂

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

3 participants