graphiti-api / graphiti

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

non-conventional primary keys and attributes #409

Open jasonkarns opened 2 years ago

jasonkarns commented 2 years ago

Hi 👋. Love that graphiti exists!

Scenario: layering in a graphiti-powered api over a legacy (non rails-conventional) database.

Our thinking is that the ActiveRecord objects should remain as close to the legacy db as possible (ie, we probably don't want to define too many alias_attributes, for instance). This way the AR models are honest about what the db schema looks like. On the flip side, the REST api should be designed to be more "ideal" in how the domain objects are represented.

To that end, it's presumably the job of the Resource then, to map the "ideal api-level" names/properties to the legacy forms on the AR models. Does this feel like the right approach without setting up for too much pain?

More particular question, what to do about non-conventional primary keys.

Assuming:

class Thing < ApplicationRecord
  self.primary_key = "thingsid"
end

Our feeling is that we would like ThingResource to still expose id as the primary key at the api layer, since that is "ideal". (And presumably, exposing non-conventional primary keys in the api layer would cause a lot of pain to the api clients and their ability to use jsonapi.org libraries.)

When we do ThingResource.find(id: 42), we get the incorrect SQL query:

SELECT `things`.* FROM `things` WHERE `things`.`id` = 42 LIMIT 1`

It seems graphiti is not respecting Thing's custom primary key. (Which also implies the active record adapter backend is not using ActiveRecord's Thing.find which would respect the customized primary key.

So, what is graphiti's opinion on the best place to define these customizations that reconciles our desire to expose the more "ideal/conventional" domain api while at the same time not setting ourselves up for a world of hurt with graphiti's resource associations and the frontend/mobile api clients (read: jsonapi spec libs). (Keeping in mind this is a legacy database so non-conventional PKeys, FKeys, and whatnot will be the rule, not the exception.)

richmolj commented 2 years ago

Hey Jason 👋 Great to hear from you!

A legacy non-rails DB was the original use case for Graphiti, actually. But we used alias_attribute a lot.

I agree this should be a Resource responsibility. But alias_attribute was easy and available so never got around to it. Ideally we'd have an alias_attribute in Graphiti itself which is more achievable than it may seem.

/things/123 is dealt the same way as /things?filter[id][eq]=123, which means under the hood it's .where(id: 123). I would alias something to id and it would "just work". But you can override all filters:

filter :id do
  eq do |scope, value|
    scope.where(thingid: value)
  end
end

This fixes your immediate issue. But for id specifically we need to make sure we're rendering the other column as well:

attribute :id do
  @object.thingid
end

Also remember it's not just eq but other operators we need to think about, depending on the type:

filter :id do
  match do |scope, value|
    scope.where("thingid LIKE %?%", value)
  end
end

Plus a whole thing when we get to writes.

So basically - there are fairly easy overrides built for this purpose, but it gets very boilerplatey when you want a comprehensive override for all behavior. So, you could do that, stick it in a module, you now have your very own Resource-based alias_attribute or api_to_db or whatever you want to call it.

That's one route, the other is to just build this into graphiti itself, lots of code changes to alias ? send(alias[name]) : send(name). This would be great to see, but not something I have the time to tackle right now.

I assume I you'd also prefer not to use a database view?

masterT commented 2 years ago

So basically - there are fairly easy overrides built for this purpose, but it gets very boilerplatey when you want a comprehensive override for all behavior. So, you could do that, stick it in a module, you now have your very own Resource-based alias_attribute or api_to_db or whatever you want to call it.

Just want to be understand, do you suggest a mixin? Including the module your are talking about in the resource class?

I have the same problem, I look at the code and I way going in the direction of creating a new adapter that inherits from the ActiveRecord adapter and override the method that return the Arel column. But this solution would only work if all the model have the same column name as primary id. I prefer what you suggest @richmolj

masterT commented 2 years ago

That's one route, the other is to just build this into graphiti itself, lots of code changes to alias ? send(alias[name]) : send(name). This would be great to see, but not something I have the time to tackle right now.

Should thoses changes only be on the adapter for ActiveRecord?

masterT commented 2 years ago

@richmolj I am available if you need help for this issue. 🙋‍♂️

richmolj commented 2 years ago

@masterT thanks! I think updating Graphiti itself is the best route, but the module maybe the quickest/easiest. If you go the Graphiti route I think it would be good to pass in the "correct" attribute to the adapters, so no change or logic needed in the adapters themselves.

masterT commented 2 years ago

All right, I will continue my digging. 🔎

jasonkarns commented 2 years ago

Finally coming back to this...

I kinda waffle on two different opinions. id is a special-case attribute that jsonapi (and graphiti) require. And ActiveRecord already exposes the ability to ask a model what it's primary key is: MyModel.primary_key returns the attribute/column name used as the pkey. So on one hand, because id is special, I think it makes sense for graphiti's AR adapter to introspect against the resource's model and figure out its primary key automatically.

On the other hand, I understand that aliasing attributes in the general case is a common activity.

IMO, I feel that hiding a database's "dirty" column names is precisely the responsibility of an ActiveRecord model, which is doing the job of the data layer. Exposing only "clean" attribute names from the model allows the api layer (resources) to be blissfully ignorant of any crufty db names. So I rather think that alias_attribute in the models is the right approach in the general case. But id being a special case for jsonapi/graphiti that it alone warrants special handling for introspection by graphiti's adapter.

thoughts?

richmolj commented 2 years ago

@jasonkarns well written, I think I agree. To do this would require touching query-building, serialization, and writes so it's a little work. But I agree with the principle.