Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change camelize behavior #293

Merged
merged 3 commits into from
May 19, 2023

Conversation

TylerPachal
Copy link
Contributor

@TylerPachal TylerPachal commented May 17, 2023

👋 I have a view which has a field which I treat as an "opaque object" which I store as a map. Something like this:

defmodule MyApp.Foobar do

  # ...
  schema "foobars" do
    field :location, :map
  end
end
defmodule MyAppWeb.FoobarView do
  use JSONAPI.View, type: "foobars"

  def fields do
    [
      :location
    ]
  end
end

In my postgres database, an example value looks something like (note the camelCased localizedAddress field):

{
  "country": "China",
  "localizedAddress": null
}

The problem is that after this data flows out of my view, the resulting payload looks like this, where the localizedAddress key has become localizedaddress:

{
    "data": {
        "attributes": {
            "location": {
                "country": "China",
                "localizedaddress": null
            }
        }
    }
}

I have pinned it down to the String module:

iex(1)> JSONAPI.Utils.String.camelize("localizedAddress")
"localizedaddress"

If changing the String module is too disruptive/risky, then I think there should be a new callback in the view that is a mechanism for giving a value for a field, but without any further transformations applied to it. I.e. today if I do this:

defmodule MyAppWeb.FoobarView do
  use JSONAPI.View, type: "foobars"

  def fields do
    [
      :location
    ]
  end
  
  # This doesn't work: whatever I do here doesn't matter because the downstream string module will still make its changes
  def location(%{location: location}, _conn) do
    location
  end

  # Maybe something like this, but with a better name
  def location_value(%{location: location}, _conn) do
    location
  end
end

@TylerPachal TylerPachal requested a review from a team as a code owner May 17, 2023 17:48
Comment on lines +97 to +98
iex> camelize("alreadyCamelized")
"alreadyCamelized"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: New test to prove an already-camelized value will remain camelized.

@@ -109,12 +112,19 @@ defmodule JSONAPI.Utils.String do
with words <-
Regex.split(
~r{(?<=[a-zA-Z0-9])[-_](?=[a-zA-Z0-9])},
to_string(value)
to_string(value),
trim: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: Using trim: true means we can remove the need to manually filter out the empty strings.

Comment on lines +119 to +121
# If there is only one word, leave it as-is
[word] ->
word
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: This is the main change. Like the comment says, if we could not split the value into multiple words (i.e. there were no - or _ delimiters) then return the single word without downcasing it.

mattpolzin
mattpolzin previously approved these changes May 18, 2023
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great; thanks! I am comfortable considering this a bug fix; I can't imagine anyone would consider the current behavior of "lowercasing when already camalized" a feature.

It would help if you bumped the version in the mix.exs file to 1.5.1 as part of this PR because it can currently take quite a while to get version-bump PRs merged which could delay us getting this fix into a release!

@TylerPachal
Copy link
Contributor Author

@mattpolzin Bumped the version! Thanks for your comment.

@mattpolzin mattpolzin merged commit 5e23b93 into beam-community:main May 19, 2023
@TylerPachal TylerPachal deleted the change-camelize-behavior branch May 19, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants