beauby / jsonapi-rails

0 stars 0 forks source link

[WIP] Initial Implementation #1

Closed NullVoxPopuli closed 4 years ago

NullVoxPopuli commented 8 years ago

Just working out thoughts in here, trying to figure out API, etc.

beauby commented 8 years ago

Looks like a promising start :+1:. I'll post some API suggestions here later.

beauby commented 8 years ago

So what I had in mind was:

hash = JSONAPI::Rails::DeserializableResource[Post].(document)
class DeserializableCreatePost < JSONAPI::Rails::DeserializableResource[Post]
  attribute :foo, required: true
  attribute :bar
  has_many :baz, types: ['foo', 'bar']
  has_one :foobar, required: true
end

hash = DeserializableCreatePost.(params)
NullVoxPopuli commented 8 years ago

JSONAPI::Rails::DeserializableResource[Post].(document)

I like this, it's the same/similar pattern as the ralis 5 migrations.

for the customized one, I like how that idea could be used in place of strong parameters -- and probably would allow for easier extension if people wanted to get funky with their JSON API create/update requests

Also, how do you feel about caching the result of JSONAPI::Rails::DeserializableResource[klass]?

Thanks!

beauby commented 8 years ago

Re. caching: sure, but that's easy once everything is in place. Also, possible alternative DSL:

class DeserializableCreatePost < JSONAPI::Rails::DeserializableResource[Post]
  required do
    attribute :foo
    has_one :foobar
  end
  optional do
    attribute :bar
    has_many :baz, types: ['foo', 'bar']
  end
end
NullVoxPopuli commented 8 years ago

I like that alternative DSL, as it more closely matches DeserializableResource from jsonapi/deserializable.

Ultimately, it's going to be a subclass either way, so the DSL can go either way (just whatever jsonapi/deserializable allows)

beauby commented 8 years ago

Yeah, plus it gives a clearer view.

beauby commented 8 years ago

I'm also pondering the benefits of having jsonapi-validations/jsonapi-deserializable be whitelist only, and delegate the actual presence (+format) validations (+type coercions) to something like dry-validation.

NullVoxPopuli commented 8 years ago

I'm all for simplification of implementation -- it allows for easier native extensions later. :-)

beauby commented 8 years ago

So the journey of an incoming payload is:

  1. ensure the document is valid (jsonapi-parser)
  2. build hash from payload (jsonapi-deserializable) – as validation would not be done upfront, there could be failures there, that have to be handled in a smart way
  3. validate obtained hash (dry-validation)

and jsonapi-rails would provide a DSL to seamlessly do at least 1 and 2.

NullVoxPopuli commented 8 years ago

Yeah for the most part -- I'd said jsonapi-rails would be a drop in replacement for just step 2 -- to keep responsibility separated and such.

beauby commented 8 years ago

Quick update here: I rewrote jsonapi-deserializable so that

  1. no more optional/required – it does whitelisting only (thus the dependency on jsonapi-validations is gone),
  2. it is built upon the principle of "if field foo is present, then add following members to the result hash" (this prevents one from building members from various attributes, but that's mainly useless anyways).

Hence, the DSL here could be as simple as:

DeserializablePost = JSONAPI::Rails::DeserializableResource[Post]
DeserializablePost.(params)
# => { ... }

which would easily be implemented as a front-end for jsonapi-deserializable.

NullVoxPopuli commented 8 years ago

excellent. I looked over your changes, and it does look really easy. I'll make some updates tomorrow morning!

NullVoxPopuli commented 8 years ago

I think this is getting to a good spot. I had to re-implement some old deserialization to make AMS's existing tests happy. But I'm not sure there is ever going to be a real world use case for the AMS testing scenarios. (where no Model object exists)

beauby commented 8 years ago

Some suggestions:

  1. Rename DeserialiazableResource to DeserializableModel (also, it may make sense to change the namespacing from JSONAPI::Rails::DeserializableModel to JSONAPI::Deserializable::Rails::Model – to be discussed)
  2. Get rid of the "unrestricted" thing. If you have no clue what you expect as incoming data, you have deeper issues than a missing deserialization layer.
beauby commented 8 years ago

Also, we need to find a way to infer the attributes/relationships only if the user does not choose to define them themselves.

beauby commented 8 years ago

The best answer to the problem mentioned above seems to be no runtime inference, but using generators instead.

beauby commented 8 years ago

$ rails generate deserializable create_user user

# app/deserializables/create_user.rb
class DeserializableCreateUser < JSONAPI::Rails::DeserializableModel
  type
  id
  attribute :foo
  attribute :bar
  has_one :foobar
  has_many :barfoo
end
beauby commented 8 years ago

After some more thoughts, renaming JSONAPI::Rails::DeserializableResource to JSONAPI::Deserializable::ActiveRecord seems to make sense. Let's discuss this when you're around @NullVoxPopuli.

beauby commented 8 years ago

Regarding the generators, I implemented them in #2.

NullVoxPopuli commented 8 years ago
  1. Rename DeserialiazableResource to DeserializableModel (also, it may make sense to change the namespacing from JSONAPI::Rails::DeserializableModel to JSONAPI::Deserializable::Rails::Model – to be discussed)

After some more thoughts, renaming JSONAPI::Rails::DeserializableResource to JSONAPI::Deserializable::ActiveRecord seems to make sense. Let's discuss this when you're around

I think I like JSONAPI::Deserializable::ActiveRecord the best :-)

  1. Get rid of the "unrestricted" thing. If you have no clue what you expect as incoming data, you have deeper issues than a missing deserialization layer.

Unrestricted is entirely for AMS -- which if we want to take deserialization testing out of AMS, then it's problem solved (which I would be in huge favor of).

Also, we need to find a way to infer the attributes/relationships only if the user does not choose to define them themselves.

I don't think this would be a problem, because the inferred attributes / relationships only take precedence if a deserializable isn't explicitly used (which would xe explicitly defined entirely, and not have inferred anything)

Regarding the generators, I implemented them in #2.

Nice work! I'll need to give it a test drive

beauby commented 8 years ago

I don't think this would be a problem, because the inferred attributes / relationships only take precedence if a deserializable isn't explicitly used (which would xe explicitly defined entirely, and not have inferred anything)

It is, because if you decide not to allow those attributes anymore, you're toast.

beauby commented 8 years ago

Regarding AMS and "unrestricted deserialization", I believe it is not an issue to get rid of it as deserialization has always been marked experimental in AMS.

NullVoxPopuli commented 8 years ago

It is, because if you decide not to allow those attributes anymore, you're toast.

how do you mean?

Regarding AMS and "unrestricted deserialization", I believe it is not an issue to get rid of it as deserialization has always been marked experimental in AMS.

excellent

beauby commented 8 years ago

I mean, if you infer an attribute :foo, then you cannot get rid of it (:foo will always be whitelisted for that deserializable, no matter what DSL method you call).

NullVoxPopuli commented 8 years ago

right, I was just saying that to solve the problem, one could inherit from

JSONAPI::Deserializable::Resource and build the deserialization however they want

NullVoxPopuli commented 8 years ago

I don't remember the status of this PR :-\

NullVoxPopuli commented 4 years ago

Cleaning up my PRs. If this work is still desired, feel free to cherry-pick