[WIP] Initial Implementation #1

NullVoxPopuli · 2016-10-19T11:44:54Z

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

beauby · 2016-10-19T12:54:24Z

Looks like a promising start 👍. I'll post some API suggestions here later.

beauby · 2016-10-19T14:07:58Z

So what I had in mind was:

anonymous/default deserialization

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

customized

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 · 2016-10-19T14:20:32Z

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 · 2016-10-19T14:27:06Z

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 · 2016-10-19T14:31:39Z

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 · 2016-10-19T14:33:28Z

Yeah, plus it gives a clearer view.

beauby · 2016-10-19T14:48:21Z

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 · 2016-10-19T14:51:39Z

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

beauby · 2016-10-19T15:06:01Z

So the journey of an incoming payload is:

ensure the document is valid (jsonapi-parser)
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
validate obtained hash (dry-validation)

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

NullVoxPopuli · 2016-10-19T15:29:33Z

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 · 2016-10-21T01:21:29Z

Quick update here: I rewrote jsonapi-deserializable so that

no more optional/required – it does whitelisting only (thus the dependency on jsonapi-validations is gone),
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 · 2016-10-21T02:44:53Z

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

beauby · 2016-10-22T14:25:17Z

lib/jsonapi.rb

@@ -0,0 +1,15 @@
+module JSONAPI
+  require_relative 'jsonapi/rails'


Why require_relative and not require?

I was using require_relative everywhere else

That does not really address the question :D

I didn't want to require the whole path jsonapi/etc for every require.

Yeah but here this is the whole path. Plus it's not like we require shittons of stuff so I'd be in favor of using require everywhere.

NullVoxPopuli · 2016-10-23T14:17:34Z

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 · 2016-10-23T14:52:31Z

lib/jsonapi/rails/deserializable_resource.rb

+        end
+      end
+
+      attr_accessor :_hash, :_options, :_klass


Why accessors?

habit, they'd be fine as attr_readers, as they are never written to outside of instantiation.

But are they needed? Why not use instance variables?

beauby · 2016-10-23T15:06:58Z

Some suggestions:

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)
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 · 2016-10-23T16:32:27Z

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

beauby · 2016-10-23T16:33:45Z

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

beauby · 2016-10-23T16:37:12Z

$ 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 · 2016-10-23T17:35:50Z

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 · 2016-10-23T19:44:34Z

Regarding the generators, I implemented them in #2.

NullVoxPopuli · 2016-10-23T19:56:16Z

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 :-)

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 · 2016-10-23T20:00:52Z

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 · 2016-10-23T20:18:50Z

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 · 2016-10-23T20:56:51Z

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 · 2016-10-23T20:58:08Z

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 · 2016-10-23T21:13:54Z

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

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

minor organization updates, spec planning add some tests minor test cleanup add more tests more testing, some cleanup - having AR class caching issues with relationsihps it just returns nil :-/ remove puts railsy result add unrestricted deserializer for backward compatibility remove unrestricted, and rename to JSONAPI::Deserializable::ActiveRecord remove unrestricted, and rename to JSONAPI::Deserializable::ActiveRecord looks like I forgot to add add polymorphic check remove unneeded attr_ settings Add generators for (de)serializable models. (#2) Revert "Add generators for (de)serializable models." (#7)

NullVoxPopuli · 2016-11-13T03:56:20Z

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

NullVoxPopuli · 2020-04-19T14:46:32Z

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

beauby reviewed Oct 22, 2016

View reviewed changes

NullVoxPopuli mentioned this pull request Oct 23, 2016

Utilize JSONAPI family of gems for deserialization. rails-api/active_model_serializers#1928

Closed

beauby reviewed Oct 23, 2016

View reviewed changes

NullVoxPopuli force-pushed the initial-implementation branch from 0344bfb to ee140da Compare October 25, 2016 16:48

loosten deps

954f2d8

NullVoxPopuli closed this Apr 19, 2020

		@@ -0,0 +1,15 @@
		module JSONAPI
		require_relative 'jsonapi/rails'

[WIP] Initial Implementation #1

[WIP] Initial Implementation #1

Conversation

NullVoxPopuli commented Oct 19, 2016

beauby commented Oct 19, 2016

beauby commented Oct 19, 2016 • edited Loading

NullVoxPopuli commented Oct 19, 2016 • edited by beauby Loading

beauby commented Oct 19, 2016

NullVoxPopuli commented Oct 19, 2016 • edited Loading

beauby commented Oct 19, 2016

beauby commented Oct 19, 2016 • edited Loading

NullVoxPopuli commented Oct 19, 2016

beauby commented Oct 19, 2016

NullVoxPopuli commented Oct 19, 2016

beauby commented Oct 21, 2016 • edited Loading

NullVoxPopuli commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Oct 23, 2016

beauby commented Oct 23, 2016

beauby commented Oct 23, 2016

beauby commented Oct 23, 2016 • edited Loading

beauby commented Oct 23, 2016

beauby commented Oct 23, 2016

NullVoxPopuli commented Oct 23, 2016 • edited Loading

beauby commented Oct 23, 2016

beauby commented Oct 23, 2016

NullVoxPopuli commented Oct 23, 2016

beauby commented Oct 23, 2016

NullVoxPopuli commented Oct 23, 2016

NullVoxPopuli commented Nov 13, 2016

NullVoxPopuli commented Apr 19, 2020

beauby commented Oct 19, 2016 •

edited

Loading

NullVoxPopuli commented Oct 19, 2016 •

edited by beauby

Loading

NullVoxPopuli commented Oct 19, 2016 •

edited

Loading

beauby commented Oct 19, 2016 •

edited

Loading

beauby commented Oct 21, 2016 •

edited

Loading

beauby commented Oct 23, 2016 •

edited

Loading

NullVoxPopuli commented Oct 23, 2016 •

edited

Loading