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

non-conventional primary keys and attributes #409

Open
jasonkarns opened this issue Apr 6, 2022 · 8 comments
Open

non-conventional primary keys and attributes #409

jasonkarns opened this issue Apr 6, 2022 · 8 comments

Comments

@jasonkarns
Copy link
Contributor

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
Copy link
Contributor

richmolj commented Apr 6, 2022

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
Copy link

masterT commented Apr 13, 2022

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
Copy link

masterT commented Apr 13, 2022

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
Copy link

masterT commented Apr 13, 2022

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

@richmolj
Copy link
Contributor

@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
Copy link

masterT commented Apr 14, 2022

All right, I will continue my digging. 🔎

@jasonkarns
Copy link
Contributor Author

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
Copy link
Contributor

richmolj commented May 5, 2022

@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.

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

3 participants