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

Attributes/Persistence not friendly to noSQL embedding #40

Open
Startouf opened this issue Jul 23, 2017 · 4 comments
Open

Attributes/Persistence not friendly to noSQL embedding #40

Startouf opened this issue Jul 23, 2017 · 4 comments

Comments

@Startouf
Copy link

Startouf commented Jul 23, 2017

As discussed in the slack, embedded noSQL relationships are not easily integratable in JsonapiCompliable module.

The current logic assumes a foreign_key is required to save associations, however Mongoid's embeds_one and embeds_many do not require the foreign key but the parent object itself. The nested objects are directly added as an attribute hash to the parent_object. It is almost as if it behaved as an attribute (with the big difference that embedded records are identified by an ID, indexable, and a special Mongoid proxy allows to use Active Model scopes on embedded relations)

In a nutshell, the resource #create, #update, and maybe even #destroy methods need a way to access the parent_object of the relation.

@Startouf
Copy link
Author

Startouf commented Jul 23, 2017

In the long term, I believe Jsonapi-suite's persistence logic needs an overhaul to be able to suppor specific persistence behaviors and not just ActiveRecord's. I was thinking of a sideposting adapter like the sideloading adapter. I also don't like that x variable in the code of JsonapiCompliable/Resource as I never understand what it represents and have to keep digging (if I understand well it somewhat stores the relation configuration). I was thinking maybe a small DSL could let us handle those specific cases in a better manner.

register_persistence_behavior_for :embeds_many do 
  it_saves :with_parent
  it_needs_parent true
end

# For ActiveRecord `belongs_to` this would look like
register_persistence_behavior_for :belongs_to do 
  it_saves :after_parent 
  it_include_attributes do |parent, child_attributes, association_config|
    # include the foreign key like in #update_foreign_key
    child_attributes[association_config.foreign_key] = parent_object.send(association_config.primary_key)
  end
  it_needs_parent false
end

And then the `it_xxx` could map to the existing code of JsonapiCompliable/Persistence logic

In the short term, passing the parent_object to the nested resource #create and #update would be sufficient. However I see no way to do this "in a clean manner" and had to bubble my monkeypatches quite the way down, and the changes introduced are quire breaking. Here's a small summary of the methods that needed some change (If I have time I'll fork & branch so we can get a better diff)

class JsonapiCompliable::Util::Persistence
  def process_has_many(relationships)
    ...
        x[:object] = x[:sideload].resource
          .persist_with_relationships(x[:meta], x[:attributes], x[:relationships],
            parent_object: x[:parent_object])
  end

  def persist_object(method, attributes, parent_object: nil)
    if parent_object.present?
      return persist_embedded_object(method, attributes, parent_object)
    end
    ....

  def persist_embedded_object(method, attributes, parent_object)
    case method
    when :destroy
      @resource.destroy(attributes[:id], parent_object)
    when :disassociate, nil
      @resource.update(attributes, parent_object)
    else
      @resource.send(method, attributes, parent_object)
    end
  end

  # Add one more argument to initialize method to store parent_object
  def initialize(resource, meta, attributes, relationships, parent_object: {})
    @resource      = resource
    @meta          = meta
    @attributes    = attributes
    @relationships = relationships
    @parent_object = parent_object
  end

 def run
    ...
    children = process_has_many(@relationships) do |x|
      if x[:sideload].type.in?([:embeds_many, :embeds_one])
        x[:parent_object] = persisted
      else
        update_foreign_key(persisted, x[:attributes], x)
      end
    end
end

class JsonapiCompliable::Resource
  # - Add @parent_object argument when calling persist_object
  def persist_with_relationships(meta, attributes, relationships,
    parent_object: nil)
    persistence = JsonapiCompliable::Util::Persistence \
      .new(self, meta, attributes, relationships, parent_object: parent_object)
    persistence.run
  end
end

As a final note, I believe what is missing to clean the code (and get rid of that x variable) would be a specific resource_context class that would hold association metadata (pretty much like what ActiveRecord or Mongoid already have). This context would be build for each association and include relevant information (such as the parent_object, the foreign_key and the primary_key)

Those are just some suggestions, anyway I'm looking for hints in case there's a better way to integrate embedded sideposting with jsonapi-suite

@Startouf
Copy link
Author

Here is a link to a compare view with my changes to make Mongoid work (I hope I didn't miss any)

https://github.com/jsonapi-suite/jsonapi_compliable/compare/master...Startouf:feature/support-mongoid-embedded?expand=1

@richmolj
Copy link
Contributor

richmolj commented Jul 30, 2017

@Startouf thanks for digging in to this.

I agree the x variable needs improving. The premise is less to store information about the resource/model, and more to store information about the request payload in a more accessible manner (the resource information is a bit more "while we're here..."). We can probably improve the Deserializer class to make this a bit more obvious and helpful.

In the meantime, here's what I believe is a simpler fix for your use case.What if we took parents from like 33, and pass it to #persist_object. We can then introspect the arity of create, update etc resource methods here passing the parents when appropriate.

This still has the downside of your parent being saved first, when you want to save things all at once. This probably means refactoring the persistence logic to have some save boolean flag.

Talking through it, though, I'm not sure this is the best approach. If you have a compound document, I would think these are more akin to attributes than associations. After all, it's not possible to fetch the "inner" relationship contents without the "outer" embedded document, right? Part of the reason this becomes difficult is this seems like it's really one object, but we're trying to treat it as several different objects.

In other words, we could put all this logic in, but you'd still have a resource that cannot save itself without its parent. This makes it different than all other resources. If something cannot save itself without a parent, it's coupled to the parent and probably a better fit for attributes. That said, I'm not super familiar with MongoDB and could definitely be off base here.

In summary, even if we could easily get this API:

def create(attributes, parents)
  # ... create ...
end

I'm not sure we should.

A better option might be a new type of relationship - something like has_embedded - instead of using the existing relationships. So different concepts are handled differently.

Thoughts?

@richmolj
Copy link
Contributor

@Startouf I ended up needing something similar for a different use case. The default case is still

def create(attrs)
end

But you can now optionally do

def create(attrs, parent = nil)
end

This applies to update and destroy as well.

You can use these lower-level methods as well if needed:

def associate(parent, child, association_name, association_type)
end

def disassociate(parent, child, association_name, association_type)
end

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