WIP: Allow :only and :except when enabling underscore_to_dash #137

wasnotrice · 2018-10-16T22:09:41Z

Fix for #132

This adds the option to specify

underscore_to_dash:
  only: [:all_my, :special_keys]

or

underscore_to_dash:
  except: [:all_my, :special_keys]

For serialization the tests are pretty solid. I haven't built out the various edge case tests for query parsing yet. Thought I'd get some feedback first.

Notes:

I modified the underscore functions to accept a view, but the view is just ignored right now. I was thinking it would be nice to be able to specify keys per-view. I couldn't come up with a great way to configure that, so that view just gets passed along, and doesn't do anything yet. But it's ready and waiting. One option would be to allow for {ViewModule, :special_key} tuples in the Application.env lists. Any thought on this?
The current behavior is that when the config specifies keys, both the key and its value gets the same treatment. So if a key is in the only list, then it and its value are run through the underscore function. If not, neither the key nor the value are. Similarly for the except list.
I haven't done any testing for the scenario where a user might specify both only and except, but if only is present, it will win, and except will be ignored.
I added some describe blocks around the tests that modify Application.env, so that we can unset the changes in an on_exit block. This makes the tests more reliable when there are failures (i.e. the Application.env doesn't leak into other tests)

snewcomer · 2018-10-27T07:22:17Z

@wasnotrice Great work! I really like the test apparatus improvements here as well. 👍

I have a proposal if you agree. First PR would be the regex modification (that we could get in quickly). Part 2 would be the rest of this PR. Let me know if you think that is feasible (I understand if no as it might be a bit hard to separate out).

wasnotrice · 2018-10-31T14:37:20Z

@snewcomer thanks for the review. I could definitely break out the regex modification (that's really just changing the regex). I'll do that and then rebase this PR once it's merged.

I realize there are a few different things happening in this PR—I tried to highlight them in the initial comment. Maybe it would be easier to review if I also broke out the improvements to test apparatus for existing tests?

Plus I see that the formatter failed. Boo :)

wasnotrice · 2018-11-02T17:18:22Z

Not sure why the formatter is failing in CI—it passes on 1.7.3

wasnotrice · 2018-11-02T18:25:30Z

This has been rebased on master. I removed the additional view parameter in underscore to focus the PR better. It should be much more straightforward now. We could add the view parameter in the future if we want to offer that level of granularity

lib/jsonapi/utils/underscore.ex

test/serializer_test.exs

doomspork · 2018-11-03T03:32:17Z

test/serializer_test.exs

+      assert attributes["metadata"] == data[:metadata]
+      assert attributes["_private_attribute"] == data[:_private_attribute]
+
+      assert Enum.find(included, fn i -> i[:type] == "user" && i[:id] == "2" end)[:attributes][


I personally find these assert statements very difficult to grok, can we clean them up in any way? It may be possible to use a pipeline:

assert "bonds" == included |> Enum.find(&(&1.type == "user" && &1.id == "2") |> get_in([:attributes, "last-name"]) # Or perhaps with a capture last_name = included |> Enum.find(&(&1.type == "user" && &1.id == "2") |> get_in([:attributes, "last-name"]) assert "bonds" == last_name

I agree! These are modeled after other tests in this file (see line 441). I thought it would be better to keep same style while adding tests, to focus on the content of this addition.

Another option would be to add some helper functions to the tests:

assert "bonds" == included |> find_user_with_id("2") |> get_attribute("last-name")

Do you want to address this here, or would it be better to do a separate PR to improve readability of these tests?

Agree with the pipeline style for these tests. And I would imagine another PR would be preferable.

lib/jsonapi/utils/underscore.ex

snewcomer

@wasnotrice I really like this PR. And thanks again for getting in the regex work. 👍

snewcomer · 2018-11-09T20:16:45Z

test/jsonapi_query_parser_test.exs

@@ -93,6 +95,8 @@ defmodule JSONAPI.QueryParserTest do
      config = struct(Config, view: MyView)
      assert parse_include(config, "author.top-posts").includes == [author: :top_posts]
      assert parse_include(config, "best-friends").includes == [:best_friends]
+      assert parse_include(config, "_private-friend").includes == [:_private_friend]
+      assert parse_include(config, "nonstandard__friend").includes == [:nonstandard__friend]


snewcomer · 2018-11-09T20:18:38Z

test/serializer_test.exs

+        :full_description,
+        :inserted_at,
+        :metadata,
+        :_private_attribute,


What are your thoughts on including these keys in the original Post view and testing that? Got lots of views in there ;)

The reason I created a new view was so that the "basic" tests could keep using the simpler view, and I wanted to make as small of an impact on existing tests as I could. But I agree, this view duplicates a lot. I'd be happy to merge them if that's preferred

lib/jsonapi/utils/underscore.ex

snewcomer · 2018-11-09T21:44:19Z

test/serializer_test.exs

+      assert attributes["metadata"] == data[:metadata]
+      assert attributes["_private_attribute"] == data[:_private_attribute]
+
+      assert Enum.find(included, fn i -> i[:type] == "user" && i[:id] == "2" end)[:attributes][


Agree with the pipeline style for these tests. And I would imagine another PR would be preferable.

snewcomer · 2018-11-09T21:45:10Z

lib/jsonapi/utils/underscore.ex

@@ -3,7 +3,7 @@ defmodule JSONAPI.Utils.Underscore do
  Helpers for replacing underscores with dashes.
  """

-  def underscore?, do: Application.get_env(:jsonapi, :underscore_to_dash, false)
+  def underscore?, do: Application.get_env(:jsonapi, :underscore_to_dash, false) != false


Should we make the config a module attribute above and use it here and down in the underscore?/1?

Sure—good idea

Well, I guess not—the module attribute doesn't work for our tests, since we change the config at runtime—I made the config into a function instead

CI checks 1.6.* formatting

snewcomer · 2018-11-17T20:38:44Z

lib/jsonapi/utils/underscore.ex

      {underscore(key), underscore(value)}
    else
      {underscore(key), value}
    end
  end
+
+  defp underscore?(key) do
+    config_specifies_underscore?(config(), key)


could we unwrap this and use config_specifies_underscore? directly?

snewcomer · 2018-11-17T20:40:41Z

Lots of really nice work here Eric!

One question that I thought I should at least ask, while hopefully not discounting the great work you did - Do we want this configuration here? The only reason I thought I would ask is the original issue pointed out a missing gap with keys with double underscores and what not. That was fixed in #139. Essentially wondering - what the strong motivation for this? More or less simply curious as I don't have a need but I'm sure others here do. 👍

wasnotrice · 2018-11-19T15:52:42Z

@snewcomer there is more to the original issue.

The original issue was that my API allows a user to store arbitrary JSON. But when they retrieve the JSON they stored, it get processed by jsonapi's underscore_to_dash setting. In this particular case, fixing the regex to ignore leading underscores does work around the issue. However, that's only a coincidence, since incoming keys tend to be camelcased, not underscored.

So, the strong motivation for this is to allow people to control the format of their jsonapi keys (underscore/dash) while also allowing them to designate content whose format should not be changed, regardless of the format of the response keys. To that end, I think the most effective place for this is not actually in the application env, but instead per-view. I think that's totally doable, and if folks agree, I could build that out either here, or in a followup PR.

I opened this PR in order to provide a more concrete way to discuss the feature, so discussing it here is totally appropriate! Although I guess it would make more sense to focus first on this sort of high-level question of appropriateness/desirability before we get deep into implementation details :)

snewcomer · 2018-11-30T18:48:07Z

lib/jsonapi/utils/underscore.ex

@@ -42,9 +42,13 @@ defmodule JSONAPI.Utils.Underscore do
      "_top__posts_"
  """
  def underscore(value) when is_atom(value) do


These method names seem confusing to me :). Really it is dasherize

:) yes I agree, they would make more sense to me if switched

snewcomer · 2018-11-30T19:02:56Z

I think generally I am hoping to get some more broad thoughts on how jsonapi should handle the spec.

From my understanding, JSONAPI defines strong conventions for how the document should look (keywords like MUST, MAY, MUST NOT). For example, here is the specification regarding member names and what it can and can't include.

Additionally, the following characters are allowed in member names, except as the first or last character:
U+002D HYPHEN-MINUS, “-“
U+005F LOW LINE, “_”
U+0020 SPACE, “ “ (not recommended, not URL safe)

https://jsonapi.org/format/#document-member-names

So my question is, what level of "strictness" should we apply? If we followed the spec, then _private_attr would be tossed out. But that would be too limiting in my opinion. On the other hand, allowing configuration of only might be too loose. i.e. you are implementing a JSONAPI compliant backend but many of your keys are not compliant? except seems to me to provide some leniency, especially when integrating multiple data sources. On the other hand, you then can put non URL safe characters in there (e.g. %).

There are escape hatches such as meta where you can put non standard information in, but that would require some finagling when an easier option would be except - https://jsonapi.org/format/#document-meta

All of this is simply discussion points and not meant to discredit any ideas here. Would love to hear some more thoughts from more ppl!

Code has changed

doomspork · 2018-12-07T20:59:53Z

Sorry @snewcomer I hadn't seen your comments here. This is a very valid discussion and I want to mull this over a bit before making a decision. I'd love to get @jeregrine's thoughts as well 😁

jherdman · 2019-01-31T20:32:41Z

@wasnotrice I was wondering if you're still interested in submitting this PR. We'd love to include it in a future release.

wasnotrice · 2019-02-04T14:42:05Z

@jherdman absolutely—I'd love to get it in when we decide how it fits into the project!

doomspork requested review from doomspork, mitchellhenke, jeregrine and snewcomer October 16, 2018 22:58

doomspork added the enhancement label Oct 16, 2018

This was referenced Nov 1, 2018

Update regexes for underscore and dash #139

Merged

Improve Application.env handling in tests #140

Merged

wasnotrice force-pushed the underscore-excludes branch from 0acc52d to 27867a9 Compare November 2, 2018 16:56

Handle underscore_to_dash except and only

436f508

wasnotrice force-pushed the underscore-excludes branch from 27867a9 to 436f508 Compare November 2, 2018 17:03

Trigger rebuild

9709aad

doomspork previously requested changes Nov 3, 2018

View reviewed changes

Changes from code review

a4bd127

snewcomer reviewed Nov 9, 2018

View reviewed changes

wasnotrice added 4 commits November 15, 2018 17:08

Format for 1.6.*

69cd673

CI checks 1.6.* formatting

Simplify config_specifies_underscore?/2

d556b0e

Extract underscore config check into a function

c06a5a1

Use valid 1.4 syntax

55b01c0

snewcomer reviewed Nov 17, 2018

View reviewed changes

snewcomer reviewed Nov 30, 2018

View reviewed changes

snewcomer mentioned this pull request Dec 1, 2018

Add camelCase member names option #142

Closed

jherdman mentioned this pull request Dec 26, 2018

Add Plug to Transform Parameters #149

Merged

mitchellhenke removed their request for review January 23, 2019 19:41

snewcomer mentioned this pull request Jan 29, 2019

Prepare for 1.0 #165

Merged

jherdman closed this Sep 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Allow :only and :except when enabling underscore_to_dash #137

WIP: Allow :only and :except when enabling underscore_to_dash #137

wasnotrice commented Oct 16, 2018

snewcomer commented Oct 27, 2018 •

edited

Loading

wasnotrice commented Oct 31, 2018

wasnotrice commented Nov 2, 2018

wasnotrice commented Nov 2, 2018

doomspork Nov 3, 2018

wasnotrice Nov 5, 2018

snewcomer Nov 9, 2018

snewcomer left a comment

snewcomer Nov 9, 2018

snewcomer Nov 9, 2018

wasnotrice Nov 15, 2018

snewcomer Nov 9, 2018

snewcomer Nov 9, 2018

wasnotrice Nov 15, 2018

wasnotrice Nov 15, 2018

snewcomer Nov 17, 2018

snewcomer commented Nov 17, 2018

wasnotrice commented Nov 19, 2018

snewcomer Nov 30, 2018

wasnotrice Nov 30, 2018

snewcomer commented Nov 30, 2018

doomspork commented Dec 7, 2018

jherdman commented Jan 31, 2019

wasnotrice commented Feb 4, 2019

WIP: Allow :only and :except when enabling underscore_to_dash #137

WIP: Allow :only and :except when enabling underscore_to_dash #137

Conversation

wasnotrice commented Oct 16, 2018

snewcomer commented Oct 27, 2018 • edited Loading

wasnotrice commented Oct 31, 2018

wasnotrice commented Nov 2, 2018

wasnotrice commented Nov 2, 2018

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

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

snewcomer commented Nov 17, 2018

wasnotrice commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Nov 30, 2018

doomspork commented Dec 7, 2018

jherdman commented Jan 31, 2019

wasnotrice commented Feb 4, 2019

snewcomer commented Oct 27, 2018 •

edited

Loading