Update Operation with Roar JSONAPI representer forces contract to open up id

[KonstantinKo]

I'm not sure whether this is an issue with trailblazer-operation, roar-jsonapi, or reform - but using them in conjunction with an Update call forces me to make the id field of a resource editable. That seems like a bad idea to me. I would prefer it to ignore the id part of the JSONAPI request.

Admittedly this is also a bit of an inconsistency with the JSONAPI spec. An update as a PATCH /resources/1 MUST include the id field. An inconsistency between the URL id and the given id in the document is not handled. Maybe there is an edge case where you would want to edit the ID. However whether you want to or not, it has to be provided as a parameter.

class MyResource < Struct.new(:id, :name)
  def self.find_by(id:)
     DB[id.to_i]
  end
  def save
    DB[id.to_i] = self
  end
end
DB = {1 => MyResource.new(1, 'one')} # Simulating database

class MyContract < Reform::Form
  property :name
end

class MyRepresenter < Roar::Decorator
  include Roar::JSON::JSONAPI.resource :resources
  attributes do
    property :name
  end
end

class MyOperation < Trailblazer::Operation
  step Model(MyResource, :find_by)
  step Contract::Build(constant: MyContract)
  step Contract::Validate(representer: MyRepresenter)
  step Contract::Persist()
end

MyOperation.({id: 1}, 'document' => '{"data":{"type":"resources","attributes":{"name":"changed"},"id":"9999"}}')
# => NoMethodError: undefined method `id=' for #<MyContract>

# Adding `property :id` to MyContract fixes the NoMethodError,
# but allows editing the ID field which is usually a security vulnerability:
MyOperation.({id: 1}, 'document' => '{"data":{"type":"resources","attributes":{"name":"changed"},"id":"9999"}}')
# => <Result:true ...>
DB
# => {1 => #<struct name="one">, 9999 => #<struct name="changed">}

[apotonick]

Hey, @myabc, I think the field id should be writeable: false?

[apotonick]

@KonstantinKo The problem lies in Reform, when it uses the representer to populate the contract. I think we have to fix the configuration in jsonapi.

[KonstantinKo]

Sounds good. property :id, writeable: false is also a good workaround for now. Thanks!

[apotonick]

Could you please open this issue in the roar-jsonapi repo and explain to @myabc what is the problem, so we can solve it for everybody? Thanks ❤️

[KonstantinKo]

Sure thing 👍 => trailblazer/roar-jsonapi#25

[myabc]

@KonstantinKo cheers. I'll try to find some time later in the week to investigate.

[panSarin]

Since issue was moved to roar-jsonapi i close that one and we will check if it is still to fix.
Also join us on zulipchat: https://trailblazer.zulipchat.com/join/od4yhihn6tra3ix39u4gks72/