Fully Support Sparse Fieldsets #171

jherdman · 2019-01-31T18:35:25Z

Whilst the QueryParser was correctly identifying requested fieldsets,
nothing was done to actually support this.

This change prunes returned fields to those requested should it be the
case.

Note that this change also includes a few more typespecs for functions I
touched or read.

Resolves #120
Closes #156

jherdman · 2019-01-31T18:40:09Z

@kbaird I think this pull request will resolve your issue with spare fieldsets, but I could use a hand in testing it. Assuming you're using the QueryParser it should "just work". Here's a sample request:

7d5cf21#diff-b3e75c65fe619484c7646cf236da6b66R281

Note that no changes to your code will be required unless you've overridden the View.attributes/2 method.

doomspork · 2019-01-31T20:00:39Z

@jherdman could you rebase this PR to remove the commits already in master? 😁

jherdman · 2019-01-31T20:11:15Z

@doomspork will do! Are you up for doing the 1.0 release first? I'm unable to do releases still.

doomspork · 2019-01-31T20:12:48Z

@jherdman for sure, I'm ready whenever we've got the team consensus 👍

lib/jsonapi/view.ex

lib/jsonapi/plugs/query_parser.ex

test/jsonapi/view_test.exs

test/jsonapi_test.exs

jherdman · 2019-01-31T21:43:55Z

@doomspork I've made some updates per your suggestions

kbaird · 2019-01-31T21:50:20Z

@jherdman I'm also trying to test and having some issues.

with {:jsonapi, github: "jherdman/jsonapi", branch: "issue-120"}, in mix.exs:

== Compilation error in file lib/inventory_service_web/router.ex ==
** (UndefinedFunctionError) function JSONAPI.PlugResponseContentType.init/1 is undefined (module JSONAPI.PlugResponseContentType is not available)
    JSONAPI.PlugResponseContentType.init([])
    (plug) lib/plug/builder.ex:302: Plug.Builder.init_module_plug/4
    (plug) lib/plug/builder.ex:286: anonymous fn/5 in Plug.Builder.compile/3
    (elixir) lib/enum.ex:1940: Enum."-reduce/3-lists^foldl/2-0-"/3
    (plug) lib/plug/builder.ex:284: Plug.Builder.compile/3
    lib/inventory_service_web/router.ex:6: (module)
    (stdlib) erl_eval.erl:680: :erl_eval.do_apply/6
    (elixir) lib/kernel/parallel_compiler.ex:208: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/6
failed

Any thoughts?

jherdman · 2019-01-31T21:53:29Z

A little while back we moved around some plugs and changed the name of that one to JSONAPI.ResponseContentType. Sorry about that!

kbaird · 2019-01-31T22:00:47Z

@jherdman I'm also getting quite a few dialyzer errors now. FYI.

kbaird · 2019-01-31T22:07:20Z

And

202) test index optionally allows filtering by product_instance_id (InventoryServiceWeb.LedgerEntryControllerTest)
     test/inventory_service_web/controllers/ledger_entry_controller_test.exs:51
     ** (FunctionClauseError) no function clause matching in JSONAPI.Utils.String.field_transformation/1

     The following arguments were given to JSONAPI.Utils.String.field_transformation/1:
     
         # 1
         nil
     
     Attempted function clauses (showing 1 out of 1):
     
         def field_transformation(transformation) when transformation === :dasherize or (transformation === :underscore or transformation === :camelize)
     
     code: conn = get(conn, path)
     stacktrace:
       (jsonapi) lib/jsonapi/utils/string.ex:209: JSONAPI.Utils.String.field_transformation/1
       (jsonapi) lib/jsonapi/serializer.ex:236: JSONAPI.Serializer.transform_fields/1
       (jsonapi) lib/jsonapi/serializer.ex:57: JSONAPI.Serializer.encode_data/4
       (jsonapi) lib/jsonapi/serializer.ex:46: anonymous fn/5 in JSONAPI.Serializer.encode_data/4
       (elixir) lib/enum.ex:1431: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
       (jsonapi) lib/jsonapi/serializer.ex:27: JSONAPI.Serializer.serialize/4
       (phoenix) lib/phoenix/view.ex:399: Phoenix.View.render_to_iodata/3
       (phoenix) lib/phoenix/controller.ex:729: Phoenix.Controller.__put_render__/5
       (inventory_service) lib/inventory_service_web/endpoint.ex:1: InventoryServiceWeb.Endpoint.instrument/4
       (phoenix) lib/phoenix/controller.ex:746: Phoenix.Controller.instrument_render_and_send/4
       (inventory_service) lib/inventory_service_web/controllers/ledger_entry_controller.ex:1: InventoryServiceWeb.LedgerEntryController.action/2
       (inventory_service) lib/inventory_service_web/controllers/ledger_entry_controller.ex:1: InventoryServiceWeb.LedgerEntryController.phoenix_controller_pipeline/2
       (inventory_service) lib/inventory_service_web/endpoint.ex:1: InventoryServiceWeb.Endpoint.instrument/4
       (phoenix) lib/phoenix/router.ex:275: Phoenix.Router.__call__/1
       (inventory_service) lib/plug/error_handler.ex:64: InventoryServiceWeb.Router.call/2
       (inventory_service) lib/inventory_service_web/endpoint.ex:1: InventoryServiceWeb.Endpoint.plug_builder_call/2
       (inventory_service) lib/inventory_service_web/endpoint.ex:1: InventoryServiceWeb.Endpoint.call/2
       (phoenix) lib/phoenix/test/conn_test.ex:235: Phoenix.ConnTest.dispatch/5

Basic GET index with filtering

jherdman · 2019-01-31T22:10:25Z

Ah, drat! Would you mind pasting your dialyzer errors into here too?

     def field_transformation(transformation) when transformation === :dasherize or (transformation === :underscore or transformation === :camelize)

This is a 1.0 change. My apologies for the bumps in the road, we're definitely doing the testing of this ticket the hard way 😁 . You'll need to configure how your fields are transformed. See: https://github.com/jeregrine/jsonapi#configuration

kbaird · 2019-01-31T22:18:11Z

field_transformation: :underscore helped. I'm still getting errors. Here's a typical one:

  1) test create product_custom_detail renders product_custom_detail when data is valid (InventoryServiceWeb.ProductCustomDetailControllerTest)
     test/inventory_service_web/controllers/product_custom_detail_controller_test.exs:118
     ** (RuntimeError) expected response with status 201, got: 400, with body:
     {"errors":[{"detail":"Check out http://jsonapi.org/format/#crud for more info.","source":{"pointer":"/data/attributes"},"status":400,"title":"Missing attributes in data parameter"}]}
     code: resp = json_response(posted_conn, 201)["data"]
     stacktrace:
       (phoenix) lib/phoenix/test/conn_test.ex:373: Phoenix.ConnTest.response/2
       (phoenix) lib/phoenix/test/conn_test.ex:419: Phoenix.ConnTest.json_response/2
       test/inventory_service_web/controllers/product_custom_detail_controller_test.exs:131: (test)

The payload worked with jsonapi 0.8.

Here's a typical dialyzer error:

lib/inventory_service_web/controllers/brand_controller.ex:1:call
The call:
JSONAPI.QueryParser.call(_ :: %Plug.Conn{_ => _}, %JSONAPI.Config{
  :data => nil,
  :fields => %{},
  :filter => [],
  :include => [],
  :opts => [
    {:filter, [<<_::32, _::size(8)>>, ...]}
    | {:include, [:brand | :organization | :products, ...]}
    | {:view, InventoryServiceWeb.BrandView},
    ...
  ],
  :page => %JSONAPI.Page{
    :cursor => nil,
    :limit => nil,
    :offset => nil,
    :page => nil,
    :size => nil
  },
  :required_fields => nil,
  :sort => nil,
  :view => InventoryServiceWeb.BrandView
})

will never return since it differs in arguments with
positions 2nd from the success typing arguments:

(
  %Plug.Conn{
    :adapter => {atom(), _},
    :assigns => %{atom() => _},
    :before_send => [(map() -> map())],
    :body_params => %Plug.Conn.Unfetched{
      :aspect => atom(),
      binary() =>
        binary() | [binary() | [any()] | map()] | %{binary() => binary() | [any()] | map()}
    },
    :cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
    :halted => _,
    :host => binary(),
    :method => binary(),
    :owner => pid(),
    :params => %Plug.Conn.Unfetched{
      :aspect => atom(),
      binary() =>
        binary() | [binary() | [any()] | map()] | %{binary() => binary() | [any()] | map()}
    },
    :path_info => [binary()],
    :path_params => %{
      binary() =>
        binary() | [binary() | [any()] | map()] | %{binary() => binary() | [any()] | map()}
    },
    :port => char(),
    :private => %{atom() => _},
    :query_params => %Plug.Conn.Unfetched{
      :aspect => atom(),
      binary() =>
        binary() | [binary() | [any()] | map()] | %{binary() => binary() | [any()] | map()}
    },
    :query_string => binary(),
    :remote_ip =>
      {byte(), byte(), byte(), byte()}
      | {char(), char(), char(), char(), char(), char(), char(), char()},
    :req_cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
    :req_headers => [{binary(), binary()}],
    :request_path => binary(),
    :resp_body =>
      nil
      | binary()
      | maybe_improper_list(
          binary() | maybe_improper_list(any(), binary() | []) | byte(),
          binary() | []
        ),
    :resp_cookies => %{binary() => %{}},
    :resp_headers => [{binary(), binary()}],
    :scheme => :http | :https,
    :script_name => [binary()],
    :secret_key_base => nil | binary(),
    :state => :chunked | :file | :sent | :set | :set_chunked | :set_file | :unset,
    :status => nil | non_neg_integer()
  },
  %JSONAPI.Config{
    :data => %{:__struct__ => atom(), atom() => _},
    :fields => map(),
    :filter => Keyword.t(),
    :include => Keyword.t(),
    :opts => _,
    :page => %JSONAPI.Page{
      :cursor => non_neg_integer(),
      :limit => non_neg_integer(),
      :offset => non_neg_integer(),
      :page => non_neg_integer(),
      :size => non_neg_integer()
    },
    :required_fields => _,
    :sort => _,
    :view => _
  }
)

jherdman · 2019-02-01T14:23:01Z

@kbaird ...

test create product_custom_detail renders product_custom_detail when data is valid

I've tracked this down to #122 . My hunch is that you're missing the "attributes" wrapper for your payload.

I'll look into that Dialyzer error. I have a hunch about what's wrong.

lib/jsonapi/view.ex

lib/jsonapi/plugs/query_parser.ex

jherdman · 2019-02-03T15:52:55Z

@kbaird I'm having trouble tracking down that Dialyzer error. I have a few ideas though:

I think you ought to try upgrading first to 0.9.0 and resolving the deprecation warnings there.
Next try 1.0.0.

My hunch is that you'll still see the Dialyzer errors. Would you mind reporting them? I think it'll help focus the remainder of this ticket.

kbaird · 2019-02-03T16:29:39Z

@jherdman Will do. It might be Monday or Tuesday. Thanks.

jherdman · 2019-02-06T14:58:07Z

lib/jsonapi/view.ex

+  ### Advanced Fields Usage
+
+  Note that the overriddable `attributes/2` method can be used for more control
+  over which fields are serialized from your view. It is **strongly** recommended


@doomspork @snewcomer I've added some documentation here about using visible_fields/2, and a bit of eager documentation about attributes/2 being deprecated at some point. Frankly I think we ought to tighten the screws on View a tad, so this may be a bit presumptuous of me. I can peel back this warning if you disagree.

Sounds good. Not sure if it is worthwhile adding why attributes may be deprecated (just for posterity sake)

Good point! I'll beef up the new note a bit.

jherdman · 2019-02-06T14:58:37Z

@snewcomer @doomspork back at ya!

kbaird · 2019-02-06T17:11:13Z

@jherdman My colleague @mjc is taking over this upgrade from me, and will report on our results. Thanks.

jherdman · 2019-02-06T17:11:55Z

Sounds great, @kbaird ! Many thanks for your time and help.

mjc · 2019-02-08T16:58:59Z

So we were able to upgrade successfully, with the one note that when you do not specify "type" in a create or update action, the error message is incorrect.

It said /data/attributes was the issue when it was actually /data/type.

jherdman · 2019-02-12T17:17:27Z

Hmm... It should have "just worked". Are you able to share your view code?

kbaird · 2019-02-12T22:14:28Z

Tried again after the force push. Same failures.

jherdman · 2019-02-12T22:15:32Z

@kbaird you're too fast ;) I think I know what's going on, but I found another bug in the process of digging into your issue. Hopefully I'll resolve your issue soon.

jherdman · 2019-02-12T23:36:54Z

lib/jsonapi/plugs/query_parser.ex

+        try do
+          value
+          |> String.split(",")
+          |> Enum.map(&underscore/1)


I think this could be considered a bug insofar that UnderscoreParameters doesn't catch this.

kbaird · 2019-02-13T03:08:41Z

I used commit: rather than branch:. Sorry, user error on my part.

jherdman · 2019-02-13T03:31:52Z

No worries! Does that mean everything is working for you now?

…

On Tue, Feb 12, 2019 at 22:08 Kevin C. Baird ***@***.***> wrote: I used commit: rather than branch:. Sorry, user error on my part. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5A_amfAASFi10YJGElNJulv7U4qWks5vM4G9gaJpZM4ac5t8> .

jherdman · 2019-02-13T03:33:23Z

Sorry, I misread your last message. That's great news! Many thanks for helping. I'll be sure to cut a release soon for you.

@doomspork I think we're good to go on this. Any last concerns before I merge this?

lib/jsonapi/plugs/query_parser.ex

lib/jsonapi/view.ex

snewcomer · 2019-02-13T04:45:26Z

lib/jsonapi/view.ex

+  ### Advanced Fields Usage
+
+  Note that the overriddable `attributes/2` method can be used for more control
+  over which fields are serialized from your view. It is **strongly** recommended


Sounds good. Not sure if it is worthwhile adding why attributes may be deprecated (just for posterity sake)

jherdman · 2019-02-13T17:01:46Z

@snewcomer I decided to pull the note on deprecating attributes/2 as an overridable method. I think I should do this in a separate PR where this is formally done.

jherdman · 2019-02-13T17:02:52Z

@doomspork @snewcomer should be ready for another round of reviewing.

README.md

jherdman · 2019-02-17T02:28:19Z

@doomspork @snewcomer any concerns with merging this?

lib/jsonapi/view.ex

jherdman · 2019-02-17T18:47:03Z

Yeah, pretty much. I kind of like that name. I'll make a change shortly.

…

On Sun., Feb. 17, 2019, 13:26 Scott Newcomer ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/jsonapi/view.ex <#171 (comment)>: > + defp fields_for_type(requested_fields, fields) when requested_fields in [nil, %{}], + do: fields + + defp fields_for_type(requested_fields, fields) do + fields + |> MapSet.new() + |> MapSet.intersection(MapSet.new(requested_fields)) + |> MapSet.to_list() + end + + @SPEC visible_fields(map(), conn :: nil | Conn.t()) :: list(atom) + def visible_fields(data, conn) do + all_fields = + conn + |> requested_fields_for_type() + |> fields_for_type(fields()) Is this something like "net fields for type"? Just trying to think of a different name if you also agreed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5A6rDIFb0ZSLZQ7TZr7wVQUHLGWsks5vOZ7SgaJpZM4ac5t8> .

jherdman · 2019-02-17T22:40:50Z

@snewcomer — renamed as requested.

snewcomer · 2019-02-18T04:26:21Z

test/jsonapi_test.exs

+               "first_character" => "H"
+             } == attributes
+    end
+  end


Last question and I swear I'm done. Is there a proper test where say excerpt is not included in the response b/c it wasn't in the fields map?

There is now 😄

Whilst the `QueryParser` was correctly identifying requested fieldsets, nothing was done to actually support this. This change prunes returned fields to those requested should it be the case. Note that this change also includes a few more typespecs for functions I touched or read. Resolves beam-community#120 Closes beam-community#156

snewcomer · 2019-02-18T15:44:19Z

lib/jsonapi/view.ex

+
+        def hidden(_data) do
+          [:email] # will be removed from the response
+        end


One thing that has stumped me so far is the need for this as a public API. Since it seems one would just modify fields to not include email. Is there a reason to have this as a public API you think?

(btw I am so sorry for going back and forth. I should have gotten these comments all rolled up into one review :()

hidden/1 comes from #126. The gist seems to be conditionally excluding fields given the data available. I think an example scenario is hiding a sensitive field from users with lower privilege.

Anyways, given that it's been part of the public API for a while we ought to leave it as such.

Oh you are right.

In the past, for lower privileges, I take advantage of checking the logic in a function for that specific field. But perhaps this allows bulk sending in fields to hide.

Documenting as such still seems weird but since it has been there 👍

snewcomer

🎈 🎉

jherdman · 2019-02-18T16:48:13Z

@doomspork can I get an amen?

jherdman added the wip label Jan 31, 2019

jherdman force-pushed the issue-120 branch 2 times, most recently from 3a37e79 to 149e44f Compare January 31, 2019 20:42

jherdman changed the title ~~Fully Support Spare Fieldsets~~ Fully Support Sparse Fieldsets Jan 31, 2019

jherdman added enhancement and removed wip labels Jan 31, 2019

doomspork requested changes Jan 31, 2019

View reviewed changes

jherdman force-pushed the issue-120 branch from 149e44f to c9eb735 Compare January 31, 2019 21:42

snewcomer reviewed Feb 2, 2019

View reviewed changes

lib/jsonapi/view.ex Show resolved Hide resolved

lib/jsonapi/plugs/query_parser.ex Outdated Show resolved Hide resolved

jherdman force-pushed the issue-120 branch from c9eb735 to c75090c Compare February 3, 2019 15:51

jherdman force-pushed the issue-120 branch from c75090c to e6bf362 Compare February 6, 2019 14:56

jherdman commented Feb 6, 2019

View reviewed changes

jherdman force-pushed the issue-120 branch from e6bf362 to ebe35a9 Compare February 12, 2019 21:09

jherdman force-pushed the issue-120 branch from ebe35a9 to 381236e Compare February 12, 2019 22:30

jherdman commented Feb 12, 2019

View reviewed changes

snewcomer reviewed Feb 13, 2019

View reviewed changes

jherdman force-pushed the issue-120 branch 2 times, most recently from 295905c to ae621db Compare February 13, 2019 17:00

jherdman force-pushed the issue-120 branch from ae621db to 5d662c1 Compare February 13, 2019 19:54

snewcomer reviewed Feb 16, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

jherdman force-pushed the issue-120 branch from 5d662c1 to 0ddb215 Compare February 17, 2019 01:07

snewcomer reviewed Feb 17, 2019

View reviewed changes

lib/jsonapi/view.ex Outdated Show resolved Hide resolved

jherdman force-pushed the issue-120 branch from 0ddb215 to 584a3bb Compare February 17, 2019 22:40

snewcomer reviewed Feb 18, 2019

View reviewed changes

jherdman force-pushed the issue-120 branch from 584a3bb to 1367099 Compare February 18, 2019 14:56

snewcomer reviewed Feb 18, 2019

View reviewed changes

snewcomer approved these changes Feb 18, 2019

View reviewed changes

jherdman merged commit de0d8af into beam-community:master Feb 21, 2019

jherdman deleted the issue-120 branch February 21, 2019 03:50

Fully Support Sparse Fieldsets #171

Fully Support Sparse Fieldsets #171

Conversation

jherdman commented Jan 31, 2019 • edited Loading

jherdman commented Jan 31, 2019

doomspork commented Jan 31, 2019

jherdman commented Jan 31, 2019

doomspork commented Jan 31, 2019

jherdman commented Jan 31, 2019

kbaird commented Jan 31, 2019

jherdman commented Jan 31, 2019

kbaird commented Jan 31, 2019

kbaird commented Jan 31, 2019

jherdman commented Jan 31, 2019

kbaird commented Jan 31, 2019

jherdman commented Feb 1, 2019

jherdman commented Feb 3, 2019

kbaird commented Feb 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherdman commented Feb 6, 2019

kbaird commented Feb 6, 2019

jherdman commented Feb 6, 2019

mjc commented Feb 8, 2019

jherdman commented Feb 12, 2019

kbaird commented Feb 12, 2019

jherdman commented Feb 12, 2019

Choose a reason for hiding this comment

kbaird commented Feb 13, 2019

jherdman commented Feb 13, 2019 via email

jherdman commented Feb 13, 2019

Choose a reason for hiding this comment

jherdman commented Feb 13, 2019

jherdman commented Feb 13, 2019

jherdman commented Feb 17, 2019

jherdman commented Feb 17, 2019 via email

jherdman commented Feb 17, 2019

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

snewcomer left a comment

Choose a reason for hiding this comment

jherdman commented Feb 18, 2019

jherdman commented Jan 31, 2019 •

edited

Loading