graphiti-api / graphiti

Stylish Graph APIs
https://www.graphiti.dev/
MIT License
976 stars 139 forks source link

Allow `attribute :id` and `filter :id` to be inherited from the root resource #151

Open wagenet opened 5 years ago

wagenet commented 5 years ago

This seems to fix it for me:

diff --git i/lib/graphiti/resource/dsl.rb w/lib/graphiti/resource/dsl.rb
index 7353edf..867fcf4 100644
--- i/lib/graphiti/resource/dsl.rb
+++ w/lib/graphiti/resource/dsl.rb
@@ -142,6 +142,7 @@ module Graphiti
         end

         def apply_attributes_to_serializer
+          return unless serializer
           serializer.type(type)
           Util::SerializerAttributes.new(self, attributes).apply
         end
richmolj commented 5 years ago

A bit of a longer explanation on this one:

So, we always want the serializer and type to inherit correctly - TopPostResource should have the same type and a subclass of the same serializer as PostResource. However, this is not true when the resource is self.abstract_class = true like ApplicationResource - we wouldn't want to inherit a type of application_resources.

So, we nil-out these properties when setting an abstract class. This is why you get an error at that line to begin with - there's no serializer because we removed it with abstract_class. If you remove that code, you'll see a ton of spec failures.

There may be a better way to work around this, but another strategy might be to raise an error telling the user to do self.inherited when trying to add an attribute, filter etc on a base class:

def self.inherited(klass)
  super
  klass.attribute(:id, :string) { @object.guid }
end
wadetandy commented 5 years ago

As this is specific to inheriting ID from the parent, we could also do this with a class attribute (on mobile so forgive formatting):

ApplicationResource self.default_id_type = :uuid end

masterT commented 2 years ago

@richmolj is @wadetandy suggestion something that could be added to the library?

masterT commented 2 years ago

I think it would be nice to be able to configure the ID attribute from an abstract resource I'm think about ::default_id_sttribute=(definition) or ::default_id_attribute(type, options = {}, &blk).

masterT commented 2 years ago

What about something like this? https://github.com/graphiti-api/graphiti/compare/master...masterT:add-resource-id-attribute-definition?expand=1