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

Relationships fall apart in many scenarios #176

Open
coreyward opened this issue Mar 26, 2016 · 7 comments
Open

Relationships fall apart in many scenarios #176

coreyward opened this issue Mar 26, 2016 · 7 comments

Comments

@coreyward
Copy link
Contributor

After a few weeks of working with JsonApiClient, I've found myself resorting to acrobatics to do straightforward things more often than I'd like. Here are a few examples that illustrate what I've run into. For these examples, consider the following simple schema:

class Order < JsonApiClient::Resource
  has_one :shipment
end

class Shipment < JsonApiClient::Resource
  belongs_to :order
end

Starting off simple: finding an order by ID.

The user needs to call first, despite the path establishing that there is no spec-compliant response that can contain more than a single resource:

Order.find(1) # returns an array
Order.where(id: 1).first # doesn't include ID in the request
Order.find(1).first # will raise JsonApiClient::Errors::NotFound if not found

Compare to how PassiveRecord handles this:

Order.find(*1,2) #=> [ <Order @id=1>, <Order @id=2> ]
Order.find(1) #=> <Order @id=1>

To get a little more complex with finders…

Chaining is great, especially when we can define scopes, a la ActiveRecord. Unfortunately, the following is required for this to work:

class Order
  class QueryBuilder < JsonApiClient::Query::Builder
    def shipped
      where(status: 'shipped')
    end
  end

  self.query_builder = Order::QueryBuilder

  # and either this:
  def self.shipped
    _new_scope.shipped
  end

  # or this: (note Forwardable is already on the Resource eigenclass)
  class << self
    def_delegators :_new_scope, :shipped
  end
end

Yikes.

What about assigning a relationship?

Here we use a simple setter to assign a pre-defined relationship (via has_one and belongs_to). Unfortunately, it doesn't do at all what's expected.

order.shipment = Shipment.new(order_id: 1)
#=> <Shipment:@attributes={"id"=>1, "type"=>"shipments"}>
order.shipment #=> nil
order.attributes #=> { "shipment" => <Shipment>, … }
order.relationships #=> not here

While the DynamicAttributes module handles the assignment methods appropriately, it isn't extended at all by relationship-aware classes that use it as a mixin. As a result, the getter method accesses the relationship, which remains nil despite the user's efforts.

Removing a resource

As a final example, when attempting to delete an associated resource as part of a compound document retrieval, the relationship traversal shortcomings become more clear:

order = Order.include(:shipment).find(1).first
order.shipment.destroy #=> ArgumentError: Not all prefix parameters specified

In this case, shipment requires order_id to build the path, but the relationship linker doesn't provide a mechanism for upward traversal, and the JSON API spec doesn't have the parent ID established in the child attributes.


I think JsonApiClient does some great things, and I'm glad it's available, but I think the above examples are a fair representation of a shortcoming that can be addressed.

I'm open to helping out in this area, provided there's interest. I considered plugging some of these gaps with smaller changes, but the Resource.relationship_linker object has a significant surface area, and the underlying implementation seems coupled with the JSON API data representation, making it hard to move on.

In closing, I have two (and a half) questions:

  1. Is there's any interest in improving the relational algebra to better suit the type of behavior demonstrated above?
  2. Does it make sense to explore integrating with PassiveRecord to make this happen, if so?
  3. Do you expect to have the time and interest to tackle this yourself, or would you be looking for a third-party contributor?

Thanks for reading through to the end!

@chingor13
Copy link
Collaborator

Thanks for taking the time to investigate all this stuff.

I am ok with improving the relationships stuff but let me first explain a few of motivations behind some of the design choices. I started this project to give a familiar interface for building json-api requests (and started it way pre-1.0). As such, some of the initial decisions were based upon the initial thinking of jsonapi.org.

Additionally, I manage 2 branches of this project - master which tracks the 1.0 spec and and 0.x branch which is the version my company uses for our internal api format. I tried to make the 1.0 gem version modular to be able to support my 0.x format in the future.

One thing that I want to impress is that services and API should abstract away most of the business logic otherwise we're coupling our API with a single client codebase. We don't want our APIs to just be a database access layer over http/rest/json.

Find

The first point about find was one of the initial design choices of jsonapi. Every response would return an array so that any client could handle the responses in the same way. In the 1.0 push, this choice was reverted to go back and follow a more restful mapping. I chose to keep returning an "array" (JsonApiClient::ResultSet) because I like that find always returns the same data type. One of my pet peeves of ActiveRecord is that find returns different things depending on what parameters you supply it.

Scopes

I don't mind trying to extend the scope interface but I don't think it's actually necessary. One of the pitfalls of building services in a SOA environment, is business logic creeping into the clients. The provided scopes map directly to the types of query building available in the jsonapi specification. In a roundabout way, it somewhat protects you from building logic into your client. I'm fine with trying to make it easier to add scopes.

Relationships

I agree that the relationship stuff is not the best. On of the features of jsonapi is that it's supposed to be somewhat of a hypermedia interface - that you can browse the relationships without previous knowledge of the structure of the endpoints. At the same time, there is some general knowledge about what base endpoints exist.

Internally, we don't use relationships or nested routes. This is because you have to know your parent's id in order to access the resource. Finding stuff by parent id could be implemented as a filter param on the resource. This does assume that you control the api source.

Because we don't use relationships, I don't have the perspective of using the relationships stuff.

Conclusion

I would definitely appreciate work on the relationships stuff. I would make sure it's not required as I don't think it should be required. I haven't actually looked into PassiveRecord. I definitely welcome collaboration on this project.

@natemacinnes
Copy link
Contributor

@coreyward @chingor13 Has any work been put into creating more robust relationship querying interface?

@coreyward
Copy link
Contributor Author

@natemacinnes I haven't done any work on it since this post was made.

@natemacinnes
Copy link
Contributor

natemacinnes commented Sep 8, 2016

@coreyward okay, thanks for the update. @chingor13 I am interested in helping out if I can. The project I am currently working on requires creating objects with nested relationships.

@JohnSmall
Copy link

@coreyward Thanks for the link to PassiveRecord. It set me thinking about using ActiveModel+PassiveRecord+Faraday to write my own JSON-API client. I wonder how hard can it be?

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
@chingor13 @JohnSmall @coreyward @natemacinnes and others