Parse `included` array from request body #13

nilsding · 2017-02-22T12:40:54Z

This is a possible solution for #10.

It extends the body options with an included block, where you can set the schema for each type.

A simplified example for the definition:

options do
  body do
    # Schema definition for the main resource
    schema(Dry::Validation.JSON { ... })

    included do
      # Schema definition for `media' type
      media(Dry::Validation.JSON { ... })
      # Schema definition for `other_types' type
      other_types(Dry::Validation.JSON { ... })
    end
  end
end

The resulting body_params then becomes an array if the included block was given with a few definitions. This array contains the main resource as the first element.

codecov-io · 2017-02-22T12:48:29Z

Codecov Report

Merging #13 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #13   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines         382    400   +18     
=====================================
+ Hits          382    400   +18

Impacted Files	Coverage Δ
lib/request_handler/base.rb	`100% <100%> (ø)`	✅
lib/request_handler/body_parser.rb	`100% <100%> (ø)`	✅
lib/request_handler/schema_parser.rb	`100% <100%> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79a08a0...747b76e. Read the comment docs.

andreaseger

generally nice just a couple small things

andreaseger · 2017-02-22T16:00:51Z

lib/request_handler/base.rb

+        schema_options:   execute_options(lookup('body.options')),
+        included_schemas: lookup('body.included')
+      ).run
+      return defaults.merge(parsed_body) unless parsed_body.is_a? Array


hmm I think I'd rather drop the support for default body then this - Not sure why we added this in the first place and if it even makes sense to have a default for that.

andreaseger · 2017-02-22T16:07:17Z

lib/request_handler/body_parser.rb

    end

    def run
-      validate_schema(flattened_request_body)
+      flattened_body = flattened_request_body


you could do here already

body, *included = flattened_request_body

if flattened_request_body returns only a hash that will end up in body otherwise the multiple return values will be split up correctly

andreaseger · 2017-02-22T16:12:55Z

lib/request_handler/body_parser.rb

    def request_body
      b = request.body
      b.rewind
      b = b.read
      b.empty? ? {} : MultiJson.load(b)
    end

-    attr_reader :request
+    def included_schemas?
+      !included_schemas.nil? && !included_schemas.empty?


small thing but I prefer

!( included_schemas.nil? || included_schemas.empty?)

andreaseger · 2017-02-22T16:15:29Z

lib/request_handler/body_parser.rb

-      body.merge!(body.delete('attributes') { {} })
-      relationships = flatten_relationship_resource_linkages(body.delete('relationships') { {} })
-      body.merge!(relationships)
+      flatten_resource!(body)


I think we could always return

[flatten_resource!(body), *parsed_included]

and do the included_schemas? check only in the parse_included

andreaseger · 2017-02-22T16:18:38Z

lib/request_handler/schema_parser.rb

@@ -14,18 +14,19 @@ def initialize(schema:, schema_options: {})

    private

-    def validate_schema(data)
+    def validate_schema(data, with: nil)


how about defining this method as

def validate_schema(data, with: schema)

andreaseger · 2017-02-22T16:26:59Z

spec/integration/schema_parser_spec.rb

+          testhandler = testclass.new(request: request)
+          expect(testhandler.to_dto)
+            .to eq(OpenStruct.new(body: [{ name: 'About naming stuff and cache invalidation' }, { view_count: 1337 }]))
+        end


this is missing the case where the schema defines some included schemas but the body doesn't have any (which is fine as the relationship entities could already be present)

andreaseger · 2017-02-22T16:33:51Z

spec/request_handler/body_parser_spec.rb

+        {
+          'type' => 'people',
+          'id' => '9',
+          'first-name' => 'Dan',


please change the example to snake_case attributes. We probably need to add some extra handling for dashed attributes. #14

andreaseger · 2017-02-22T16:35:42Z

spec/request_handler/body_parser_spec.rb

  end
  shared_examples 'flattens the body as expected' do
-    it 'returns the flattend body' do
-      expect(handler).to receive(:validate_schema).with(wanted_result)
+    it 'returns the flattened body' do


I'd prefer if we either have a separate shared_example for the case with included schemas or - as it's only a single example right now - move this handling directly there

andreaseger · 2017-02-22T16:36:36Z

README.md

+  # The first object is the main resource object, i.e. the one that is about to
+  # be created
+  {
+    :id      => '1',


please use the new hash syntax in the example

andreaseger · 2017-02-22T16:37:41Z

README.md

@@ -90,17 +90,17 @@ class DemoHandler < RequestHandler::Base
      options(->(_handler, _request) { { foo: "bar" } })
      # options({foo: "bar"}) # also works for hash options instead of procs
    end
+  end


runtasticBot · 2017-02-22T20:40:32Z

	1 Warning
⚠️	Please limit commit subject line to 50 characters. `63c5e9f`

Generated by 🚫 Danger

andreaseger · 2017-02-22T22:07:40Z

lib/request_handler/schema_parser.rb

      validation_failure?(validator)
      validator.output
    end

-    def validate(data)
+    def validate(data, with:)


I think naming wise it would be more clear if we call the with parameter schema or validation_schema. Especially the else case below read weird with.with

andreaseger · 2017-02-22T22:08:52Z

spec/request_handler/body_parser_spec.rb

+    end
+    it 'flattens the body as expected' do
+      expect(handler).to receive(:validate_schema).with(wanted_result.shift)
+      wanted_result.each { |result| expect(handler).to receive(:validate_schema).with(result, with: anything) }


I think a do/end block would read nicer

Fix usage example in README

3f086a1

nilsding requested a review from andreaseger February 22, 2017 12:40

andreaseger requested changes Feb 22, 2017

View reviewed changes

nilsding and others added 3 commits February 22, 2017 21:11

Parse included array from request body

be4c9d4

Drop support for default request body

c3008ea

Move example specific for included schemas to the context

63c5e9f

nilsding force-pushed the master branch from 061400c to ec6318c Compare February 22, 2017 20:39

andreaseger requested changes Feb 22, 2017

View reviewed changes

nilsding and others added 2 commits February 23, 2017 08:06

Add specs for testing `included' array in body

6e02ff4

Rename with' keyword arg to schema'

c591a79

nilsding force-pushed the master branch from ec6318c to c591a79 Compare February 23, 2017 07:24

Raise error if included was given but not defined

747b76e

andreaseger approved these changes Feb 28, 2017

View reviewed changes

andreaseger merged commit f3b390b into runtastic:master Feb 28, 2017

nilsding mentioned this pull request Feb 28, 2017

Merge included array in request body with relationships #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse `included` array from request body #13

Parse `included` array from request body #13

nilsding commented Feb 22, 2017

codecov-io commented Feb 22, 2017 •

edited

Loading

andreaseger left a comment

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

runtasticBot commented Feb 22, 2017 •

edited

Loading

andreaseger Feb 22, 2017

andreaseger Feb 22, 2017

Parse included array from request body #13

Parse included array from request body #13

Conversation

nilsding commented Feb 22, 2017

codecov-io commented Feb 22, 2017 • edited Loading

Codecov Report

andreaseger left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runtasticBot commented Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parse `included` array from request body #13

Parse `included` array from request body #13

codecov-io commented Feb 22, 2017 •

edited

Loading

runtasticBot commented Feb 22, 2017 •

edited

Loading