graphiti-api / graphiti

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

Decide if `id` should be a required method on resolved objects #152

Open wagenet opened 5 years ago

wagenet commented 5 years ago

Right now if it isn't there's an error at https://github.com/graphiti-api/graphiti/blob/master/lib/graphiti/debugger.rb#L53

richmolj commented 5 years ago

I think we do have to require this, though we could probably better document. You'll actually have a problem after the debugger during serialization. jsonapi-rb requires the id in order to populate the required id section in the JSON:API payload. In addition, if you have 10 records with the same non-unique ID it will only render 1 record.

I can imagine something like SecureRandom.uuid unless @object.respond_to?(:id), but I think I lean against the implicitness at this point.

wagenet commented 5 years ago

Makes sense. Maybe we should at least make it error earlier if there isn't an id defined?

richmolj commented 5 years ago

I think we could certainly raise a better error. One way to do this would be override the serializer id attribute and raise a better error when being accessed. If we wanted to raise it earlier, a good spot might be Resource#decorate_record, which happens after we resolve models but before serialization https://github.com/graphiti-api/graphiti/blob/master/lib/graphiti/resource.rb#L27-L33

sandstrom commented 3 years ago

I'm doing some issue gardening 🌱🌿 🌷 and came upon this issue. Since it's quite old I just wanted to ask if this is still relevant? If it isn't, maybe we can close this issue?

By closing some old issues we reduce the list of open issues to a more manageable set.

richmolj commented 3 years ago

Super old but a persistent issue I think we should keep

jasonkarns commented 2 years ago

Definitely old, but after a few months of using Graphiti, I rather lean toward having a generated ID (or at least configurable option).

My rationale is that for resources backed by non-persisted models, we're required to add @id to the model instance. However, the fact that an id is required is an api level concern. (required by the spec). In its ideal form, I would think that Api requirements should not force anything upon the data or domain layers. But in its present form, jsonapi's id requirement is forcing behavior into the domain and data layer.

I wouldn't be so in favor of an implicit id, however, if one could trivially define a generated id at the resource layer. Unfortunately:

attribute :id, :uuid do
  @id ||= SecureRandom.uuid
end

isn't enough to prevent a NoMethodError id on the model instance.