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

Resource#find Returns Arrays #75

Closed
jakesower opened this issue Jul 8, 2015 · 9 comments
Closed

Resource#find Returns Arrays #75

jakesower opened this issue Jul 8, 2015 · 9 comments

Comments

@jakesower
Copy link
Contributor

I am curious why .first is is required in this snippet (from the README):

u = MyApi::Person.find(1).first
u.update_attributes(
  a: "b",
  c: "d"
)

It makes more sense to me to do something like:

u = MyApi::Person.find(1)
u.update_attributes(
  a: "b",
  c: "d"
)

This is intentional behavior: https://github.com/chingor13/json_api_client/blob/master/lib/json_api_client/parsers/parser.rb#L62

I will be changing this logic in my fork of the repository to make it work like the second snippet above. I would be interested in the thinking behind that choice, and/or if there would be interest in merging the change from my branch into this one.

@chingor13
Copy link
Collaborator

This was intentional for a few reasons:

First, I always wanted to make a result set be available because there is extra meta data that comes back with the response. We could embed the meta data in a single resource object but that seems weird because it wouldn't be available on the resource objects fetched in a multi object response. I guess we could store a reference to the meta data but that also seems a bit janky.

Second, in the initial version of jsonapi, all responses returned arrays - even requests for a single resource. The rationale behind that was to make client side parsing easier - no matter what you were fetching, you could always iterate of the results like an array. There was a change (about a year ago?) to the spec that brought back the ability to respond with a single object or and array of objects as the main data. I wasn't a huge fan of it at the time.

Additionally, I really don't like the behavior in ActiveRecord where find can return a single result or an array depending on what input you give it. This departs from ActiveRecord but I'm ok with that here.

Hope this gives some insight into why it's doing this.

@chingor13
Copy link
Collaborator

I would be open to doing away with find completely for a more explicit interface for fetching by a params hash vs. primary key vs. an array of primary keys. I'm not sure how that would look or how you'd get to the response meta data though.

@Fivell
Copy link
Contributor

Fivell commented Jul 8, 2015

there can be another method for easy fetching single resource, changing find behaviour is breaking change , don't think is good idea.
My suggestion is to use another shortcut method something like load(id) , first(id) or whatever for this purposes.

@jakesower
Copy link
Contributor Author

Thanks for taking the time to get back to me on this.

I'm looking to mimic the API's way of representing data more closely. Right now the find method does double duty: first, it gets the actual response, but then it flattens the attributes into that response. This makes it more convenient to use the result right away, but it blurs the line between primary data and metadata.

Ideally, there would be a clean way of separating the two while preserving ease of use. I see SomeResource.find as being something that's supposed to return resources to me--and resources shouldn't inherently have response metadata on them.

It is clearly a breaky change, and a different name might be best way to go.

@nlsrchtr
Copy link

I'm wondering why this issue was closed, since I can't find any method offering the suggested behaviour. Currently I'm using my own QueryBuilderwith:

class My::QueryBuilder < ::JsonApiClient::Query::Builder
  def find(args = {})
    args.is_a?(Hash) ? super : super.first
  end
end

Maybe this helps someone else or @jakesower.

@chingor13
Copy link
Collaborator

The way it works now, find will always return a JsonApiClient::ResultSet. You may want access to the meta data from the request. It is one of the things that I don't like in ActiveRecord - that find can return an array or a single AR instance depending on the arguments.

It's possible to add a find_one(args) or first(id) to make it more clear and avoid confusion stemming from the difference from ActiveRecord.

@nlsrchtr
Copy link

@chingor13 thanks for your feedback. I would like to give you my thoughts here as well.

I see your point in consistency with find. I see it the way around: I expect find to always return just one single resource. That's my expectation on this method. I rarely (aka never) trying to get multiple resources with find with the behavior, that it raises ActiveRecord::RecordNotFound if one of the ids is referring to a missing resource. I would always use where if I'm querying for multiple resources.

Since I'm requesting the model, I expect to get the model and not some metadata with it. I would need it only in very rare situations. For me it would make more sense to not have the metadata returned per default, but the model-data. So methods like find_with_metadata would make more sense in my ruby world. The data of the model is having a higher priority, from my point of view.

And not to forget: Thanks for your time and work you put into this gem!

@sringling
Copy link

You may want to use json_api_resource in conjunction with json_api_client. It provides that find behavior and allows custom methods on your API objects on the application side.

@nlsrchtr
Copy link

nlsrchtr commented Mar 3, 2016

@sringling: I'm using json_api_resource in combination with json_api_client but I don't see your point in my described find behavior?

@chingor13 Do you see any chance to make find return the resource directly and find_with_metadata the current combination of resource- and meta data?

BTW: Would be great if you could reopen the issue, since there is still no easy solution.

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

5 participants