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.

Sign up for GitHub

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

Don't Crash on Map Update #22

Merged
merged 5 commits into from
Oct 31, 2024
Merged

Don't Crash on Map Update #22

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edc024b
map.update! -> map.update
kyleVsteger Oct 31, 2024
d2d6e0f
add test that exercises new code
kyleVsteger Oct 31, 2024
4ead28b
format
kyleVsteger Oct 31, 2024
6e16044
remove helper module
kyleVsteger Oct 31, 2024
3fe90cc
make mod_base simpler
kyleVsteger Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/channel_spec/schema.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ defmodule ChannelSpec.Schema do
refs ->
schema = ref.schema()
refs = Map.put(refs, ref, schema)
refs = Map.update!(refs, :__unresolved_refs, &(&1 -- [ref]))
refs = Map.update(refs, :__unresolved_refs, [], &(&1 -- [ref]))

{schema, refs} = compile_refs(schema, refs, [])

Expand Down
7 changes: 6 additions & 1 deletion mix.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ defmodule ChannelSpec.MixProject do
elixir: "~> 1.13",
deps: deps(),
docs: docs(),
package: package()
package: package(),
elixirc_paths: elixirc_paths(Mix.env())
]
end

Expand Down Expand Up @@ -51,4 +52,8 @@ defmodule ChannelSpec.MixProject do
formatters: ["html"]
]
end

# Specifies which paths to compile per environment.
defp elixirc_paths(:test), do: ["lib", "test/support"]
defp elixirc_paths(_), do: ["lib"]
end
77 changes: 77 additions & 0 deletions test/channel_spec/socket_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
use ChannelSpec.Operations
end

defmodule :"#{mod}" do

Check warning on line 39 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test2339)

Check warning on line 39 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test2146)
use ChannelSpec.Socket

channel("foo", __MODULE__.MyChannel, assigns: [foo: "bar"])
Expand Down Expand Up @@ -82,7 +82,7 @@
use ChannelSpec.Operations
end

defmodule :"#{mod}" do

Check warning on line 85 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test932)
use ChannelSpec.Socket

channel("foo", __MODULE__.MyChannel)
Expand All @@ -107,7 +107,7 @@
use ChannelSpec.Operations
end

defmodule :"#{mod}" do

Check warning on line 110 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test1602)

Check warning on line 110 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test2755)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -169,7 +169,7 @@
subscription "foo", %{type: :string}
end

defmodule :"#{mod}" do

Check warning on line 172 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test101)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -197,7 +197,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 200 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test931)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -239,7 +239,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 242 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test483)

Check warning on line 242 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test99)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -285,7 +285,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 288 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test804)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -323,7 +323,7 @@
handle "bar", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 326 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test1604)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -367,7 +367,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 370 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test4004)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -408,7 +408,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 411 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 24.3) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test2435)

Check warning on line 411 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test1732)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -453,7 +453,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 456 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test2179)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -504,7 +504,7 @@
handle "foo", fn _params, _context, socket -> {:noreply, socket} end
end

defmodule :"#{mod}" do

Check warning on line 507 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test3074)
use ChannelSpec.Socket

channel "foo", __MODULE__.MyChannel
Expand Down Expand Up @@ -547,7 +547,7 @@
subscription "sub", %{type: :integer}
end

defmodule :"#{mod}" do

Check warning on line 550 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.13.4, 25.0.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module Test4100)
use ChannelSpec.Socket, schema_path: Path.join(tmp_dir, "schema.json")

channel "foo", __MODULE__.MyChannel, schema_file: Path.join(tmp_dir, "schema.json")
Expand Down Expand Up @@ -595,5 +595,82 @@

refute File.exists?(Path.join(tmp_dir, "schema.json"))
end

@tag :tmp_dir
test "schemas nested in a single module with references work", %{
mod: mod,
tmp_dir: tmp_dir
} do
defmodule LotsOfRefsSchema.MyChannel do
use ChannelHandler.Router
use ChannelSpec.Operations

operation "foo", payload: %{"$ref": LotsOfRefsSchema.Base}
handle "foo", fn _params, _context, socket -> {:noreply, socket} end

subscription "sub", %{type: :integer}
end

defmodule LotsOfRefsSchema do

Check warning on line 614 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

redefining module ChannelSpec.SocketTest.LotsOfRefsSchema (current version loaded from _build/test/lib/channel_spec/ebin/Elixir.ChannelSpec.SocketTest.LotsOfRefsSchema.beam)

Check warning on line 614 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.14.3, 25.2) 

function id/1 required by behaviour Phoenix.Socket is not implemented (in module ChannelSpec.SocketTest.LotsOfRefsSchema)

Check warning on line 614 in test/channel_spec/socket_test.exs

View workflow job for this annotation

GitHub Actions / Elixir Unit Tests (1.15.5, 26) 

redefining module ChannelSpec.SocketTest.LotsOfRefsSchema (current version loaded from _build/test/lib/channel_spec/ebin/Elixir.ChannelSpec.SocketTest.LotsOfRefsSchema.beam)
use ChannelSpec.Socket, schema_path: Path.join(tmp_dir, "schema.json")

channel "foo", __MODULE__.MyChannel, schema_file: Path.join(tmp_dir, "schema.json")
end

saved_json = Path.join(tmp_dir, "schema.json") |> File.read!() |> Jason.decode!()

auto_assert %{
"channels" => %{
"foo" => %{
"messages" => %{
"foo" => %{"payload" => %{"$ref" => "#/definitions/Base"}}
},
"subscriptions" => %{"sub" => %{"type" => "integer"}}
}
},
"definitions" => %{
"Bar" => %{
"properties" => %{"baz" => %{"$ref" => "#/definitions/Baz"}},
"type" => "object"
},
"Base" => %{
"additionalProperties" => false,
"properties" => %{
"bar" => %{
"items" => [%{"$ref" => "#/definitions/Bar"}],
"type" => "array"
},
"flim" => %{
"items" => [%{"$ref" => "#/definitions/Flim"}],
"type" => "array"
},
"foo" => %{"$ref" => "#/definitions/Foo"}
},
"type" => "object"
},
"Baz" => %{"oneOf" => [%{"type" => "string"}, %{"type" => "null"}]},
"Flam" => %{
"additionalProperties" => false,
"properties" => %{
"whatever" => %{
"items" => [%{"items" => [%{"type" => "string"}], "type" => "array"}],
"type" => "array"
}
},
"type" => "object"
},
"Flim" => %{
"additionalProperties" => false,
"properties" => %{
"flam" => %{
"oneOf" => [%{"type" => "null"}, %{"$ref" => "#/definitions/Flam"}]
}
},
"type" => "object"
},
"Foo" => %{"oneOf" => [%{"type" => "null"}, %{"type" => "string"}]}
}
} <- saved_json
end
end
end
76 changes: 76 additions & 0 deletions test/support/test_schema.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
defmodule ChannelSpec.SocketTest.LotsOfRefsSchema do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest creating modules in-place in the tests instead of helper modules, like here: https://github.com/felt/channel_spec/blob/main/test/channel_spec/socket_test.exs#L51-L69

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 ! Any specific reason? I broke it out since it was 75+ lines of code and that seemed like a lot to put in a test

Copy link
Contributor

@doorgan doorgan Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I find it easier to have the whole setup in the test itself in macro heavy code to both have the full picture and making sure unrelated code is not affecting the generated code.
It's a matter of preference, but macros don't always produce helpful stacktraces so I'd rather avoid sharing modules in tests, and at least have the certainty that I know exactly which test is causing issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. However, the nested nature of the setup for this test doesn't allow us to make nested modules with refs. Example:

test "foo", %{mod: mod} do 
    # I can define the top level name
    defmodule :"#{mod}.LotsOfRefsSchema" do
      # And I can create modules in this namespace
      defmodule Base do
        def schema() do
          %{
            type: :object,
            properties: %{
              # But I can't reference it here — #{mod} is not available
              foo: %{"$ref": :"#{mod}.LotsOfRefsSchema.Foo"},
            },
            additionalProperties: false
          }
        end
      end

      defmodule Foo do
        def schema() do
          %{type: :object}
        end
      end
    end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: undefined variable "mod"
  test/channel_spec/socket_test.exs:613: Test776.LotsOfRefsSchema.Base.schema/0
     ** (CompileError) test/channel_spec/socket_test.exs: cannot compile module Test776.LotsOfRefsSchema.Base (errors have been logged)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's a good point. You could move the defmodule Foo inside defmodule Base and use __MODULE__.Foo instead, or not nest them(you'd repeat .LotsofRefsSchema quite a bit) but it's a good point still.

I'm fine with either approach seeing a helper module may be easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit. I was able to get it to work with a @mod_base Module.split(__MODULE__) |> Enum.drop(-1) |> Module.concat()

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice trick! 💜

@moduledoc """
This specific setup causes `__unresolved_refs` to get into a state that makes the schema
fail to compile. The PR in which this file was introduced switches from `Map.update!` to
`Map.update` with a default and generates a valid schema.
"""

defmodule Base do
@behaviour ChannelSpec.Schema

def schema() do
%{
type: :object,
properties: %{
foo: %{"$ref": ChannelSpec.SocketTest.LotsOfRefsSchema.Foo},
bar: %{type: :array, items: [%{"$ref": ChannelSpec.SocketTest.LotsOfRefsSchema.Bar}]},
flim: %{ type: :array, items: [%{"$ref": ChannelSpec.SocketTest.LotsOfRefsSchema.Flim}] }
},
additionalProperties: false
}
end
end

defmodule Flim do
@behaviour ChannelSpec.Schema

def schema() do
%{
type: :object,
properties: %{
flam: %{
oneOf: [
%{type: :null},
%{"$ref": ChannelSpec.SocketTest.LotsOfRefsSchema.Flam}
]
}
},
additionalProperties: false
}
end
end

defmodule Foo do
def schema() do
%{ oneOf: [ %{type: :null}, %{type: :string} ] }
end
end

defmodule Bar do
def schema() do
%{type: :object, properties: %{baz: %{"$ref": ChannelSpec.SocketTest.LotsOfRefsSchema.Baz}}}
end
end

defmodule Baz do
def schema() do
%{oneOf: [%{type: :string}, %{type: :null}]}
end
end
end

defmodule ChannelSpec.SocketTest.LotsOfRefsSchema.Flam do
def schema() do
%{
type: :object,
properties: %{
whatever: %{
type: :array,
items: [ %{ type: :array, items: [%{type: :string}] }
]
}
},
additionalProperties: false
}
end
end
Loading