ash-project / ash_json_api

The JSON:API extension for the Ash Framework
https://hexdocs.pm/ash_json_api
MIT License
56 stars 40 forks source link

fix: Remove id from resource attributes #80

Closed skanderm closed 1 year ago

skanderm commented 1 year ago

id is currently being included in the attributes list in the schema (and is also being required), where it's otherwise being skipped during serialization: https://github.com/ash-project/ash_json_api/blob/main/lib/ash_json_api/serializer.ex#L599

The spec doesn't seem to include id in attributes, but it's definitely not required: https://jsonapi.org/format/#document-resource-objects

zachdaniel commented 1 year ago

Interesting...I think what we may need to do is remove the id field if and only if it's the only primary key attribute. We handle composite primary keys, for example. I'll need to double check the logic for how we hide the id field but if it isn't, it should be smarter than rejecting where name == id IMO

skanderm commented 1 year ago

Got it, that makes sense. Right now the id used is pulled or constructed here: https://github.com/ash-project/ash_json_api/blob/main/lib/ash_json_api/serializer.ex#L391

I think at the very least, id shouldn't be part of required attributes. For a project I'm working on, we're using an OpenAPI client generator and it was the required id attribute that created problems.

zachdaniel commented 1 year ago

Yeah, that makes sense. So at the moment this is the operative code in the serializer: https://github.com/ash-project/ash_json_api/blob/main/lib/ash_json_api/serializer.ex#L599

Would you mind abstracting that out to a helper, call it reject_id or something, and then use it in both places? That way later when we want to make it smarter we have one place to change it. And I guess while you're at it the best thing would be to only hide the :id field if it is also a primary_key?: true.

skanderm commented 1 year ago

Sure, happy to give it a try!

skanderm commented 1 year ago

I added a simple function and a test for rejecting ids. In the future, might something like this work?

def primary_key?(resource, field) do
  resource
  |> Ash.Resource.Info.primary_key()
  |> case do
    [^field] -> true
    _ -> false
  end
end
zachdaniel commented 1 year ago

I like it. Maybe let's call it only_primary_key?. I.e it's the only attribute that is primary key.

skanderm commented 1 year ago

Great! I went ahead with that. Do you still want the simple check in there or will this be ok?