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

Add constraints phase #4

Merged
merged 7 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
11 changes: 11 additions & 0 deletions lib/constraints.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
defmodule AbsintheHelpers.Constraint do
@moduledoc false

alias Absinthe.Blueprint.Input

@type error_reason :: atom()
@type error_details :: map()

@callback call(Input.Value.t(), tuple()) ::
{:ok, Input.Value.t()} | {:error, error_reason(), error_details()}
end
27 changes: 27 additions & 0 deletions lib/constraints/max.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
defmodule AbsintheHelpers.Constraints.Max do
@moduledoc false

@behaviour AbsintheHelpers.Constraint

def call(node = %{items: _items}, {:max, _min}) do
{:ok, node}
end

def call(node = %{data: data = %Decimal{}}, {:max, max}) do
if is_integer(max) and Decimal.gt?(data, max),

Choose a reason for hiding this comment

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

Also I do wonder if we need to use the Decimal here 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Remember that these constraint values are reflected in the GraphQL schema. I'm unsure how we should represent decimals in that context. Should we represent them as strings and parse them here, or as floats? In the Surgex parsers, we compared decimals against integers, maybe we'll be fine here as well, this is reversible too

do: {:error, :max_exceeded, %{max: max, value: data}},
else: {:ok, node}
end

def call(node = %{data: data}, {:max, max}) when is_binary(data) do

Choose a reason for hiding this comment

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

How would the Constraints directive work in conjunction with Transformation? e.g If I've IDs in string format that I would like to transform to integers but also use the min/max constraints, what would be the order of execution? Is it transformation first and then constraints or do we get to set the order we like?

Copy link
Member Author

Choose a reason for hiding this comment

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

we decide on the order, it depends on the order its added to the pipeline, in our case I've added constraints then transforms here:

https://github.com/surgeventures/system/blob/e321afcf26677b0dc40b86d5a37f740422ef0d81/apps/shedul-umbrella/apps/partners_api_gql/lib/partners_api_gql/router.ex#L44-L49

Copy link
Member Author

Choose a reason for hiding this comment

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

if String.length(data) > max,
do: {:error, :max_exceeded, %{max: max, value: data}},
else: {:ok, node}
end

def call(node = %{data: data}, {:max, max}) do
if data > max,
do: {:error, :max_exceeded, %{max: max, value: data}},
else: {:ok, node}

Choose a reason for hiding this comment

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

Suggested change
if String.length(data) > max,
do: {:error, :max_exceeded, %{max: max, value: data}},
else: {:ok, node}
end
def call(node = %{data: data}, {:max, max}) do
if data > max,
do: {:error, :max_exceeded, %{max: max, value: data}},
else: {:ok, node}
if String.length(data) > max,
do: {:error, :max_exceeded, %{max: max}},
else: {:ok, node}
end
def call(node = %{data: data}, {:max, max}) do
if data > max,
do: {:error, :max_exceeded, %{max: max}},
else: {:ok, node}

I would be against passing values here. I'm always afraid, that if that's a struct we will have problems with serializing those. But what worries me more, is that we are putting into error parameters send to us from users. Some services are putting those errors into logs (all returned errors from graplQL - since we don't really have error codes...) and it could lead to leaking of confidential information :/

Choose a reason for hiding this comment

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

(and that goes for all values added to errors)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, removed user values from errors ✅

end
end
17 changes: 17 additions & 0 deletions lib/constraints/max_items.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
defmodule AbsintheHelpers.Constraints.MaxItems do
@moduledoc false

@behaviour AbsintheHelpers.Constraint

def call(node = %{items: items}, {:max_items, max_items}) do
if Enum.count(items) > max_items do
{:error, :max_items_exceeded, %{max_items: max_items, items: Enum.map(items, & &1.data)}}
else
{:ok, node}
end
end

def call(node, {:max_items, _max_items}) do
{:ok, node}
end
end
27 changes: 27 additions & 0 deletions lib/constraints/min.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
defmodule AbsintheHelpers.Constraints.Min do
@moduledoc false

@behaviour AbsintheHelpers.Constraint

def call(node = %{items: _items}, {:min, _min}) do
{:ok, node}
end

def call(node = %{data: data = %Decimal{}}, {:min, min}) do
if is_integer(min) and Decimal.lt?(data, min),
do: {:error, :min_not_met, %{min: min, value: data}},
else: {:ok, node}
end

def call(node = %{data: data}, {:min, min}) when is_binary(data) do
if String.length(data) < min,
do: {:error, :min_not_met, %{min: min, value: data}},
else: {:ok, node}
end

def call(node = %{data: data}, {:min, min}) do
if data < min,
do: {:error, :min_not_met, %{min: min, value: data}},
else: {:ok, node}
end
end
17 changes: 17 additions & 0 deletions lib/constraints/min_items.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
defmodule AbsintheHelpers.Constraints.MinItems do
@moduledoc false

@behaviour AbsintheHelpers.Constraint

def call(node = %{items: items}, {:min_items, min_items}) do
if Enum.count(items) < min_items do
{:error, :min_items_not_met, %{min_items: min_items, items: Enum.map(items, & &1.data)}}
else
{:ok, node}
end
end

def call(node, {:min_items, _min_items}) do
{:ok, node}
end
end
75 changes: 75 additions & 0 deletions lib/directives/constraints.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
defmodule AbsintheHelpers.Directives.Constraints do
@moduledoc """
Defines a GraphQL directive for adding constraints to fields and arguments.

Supports:
- `:min`, `:max`: For numbers and string lengths
- `:min_items`, `:max_items`: For lists

Applicable to scalars (:string, :integer, :float, :decimal) and lists.

Example:
field :username, :string, directives: [constraints: [min: 3, max: 20]]
arg :tags, list_of(:string), directives: [constraints: [max_items: 5, max: 10]]

Constraints are automatically enforced during query execution.
"""

use Absinthe.Schema.Prototype

alias Absinthe.Blueprint.TypeReference.{List, NonNull}

@constraints %{
string: [:min, :max],
number: [:min, :max],
list: [:min, :max, :min_items, :max_items]
}

directive :constraints do
on([:argument_definition, :field_definition])

arg(:min, :integer, description: "Minimum value allowed")
arg(:max, :integer, description: "Maximum value allowed")
arg(:min_items, :integer, description: "Minimum number of items allowed in a list")
arg(:max_items, :integer, description: "Maximum number of items allowed in a list")

expand(&__MODULE__.expand_constraints/2)
end

def expand_constraints(args, node = %{type: type}) do
do_expand(args, node, get_args(type))
end

defp get_args(:string), do: @constraints.string
defp get_args(type) when type in [:integer, :float, :decimal], do: @constraints.number
defp get_args(%List{}), do: @constraints.list
defp get_args(%NonNull{of_type: of_type}), do: get_args(of_type)
defp get_args(type), do: raise(ArgumentError, "Unsupported type: #{inspect(type)}")

defp do_expand(args, node, allowed_args) do
{valid_args, invalid_args} = Map.split(args, allowed_args)
handle_invalid_args(node, invalid_args)
update_node(valid_args, node)
end

defp handle_invalid_args(_, args) when map_size(args) == 0, do: :ok

defp handle_invalid_args(%{type: type, name: name, __reference__: reference}, invalid_args) do
args = Map.keys(invalid_args)
location_line = get_in(reference, [:location, :line])

raise Absinthe.Schema.Error,
phase_errors: [
%Absinthe.Phase.Error{
phase: __MODULE__,
message:
"Invalid constraints for field/arg `#{name}` of type `#{inspect(type)}`: #{inspect(args)}",
locations: [%{line: location_line, column: 0}]
}
]
end

defp update_node(args, node) do
%{node | __private__: Keyword.put(node.__private__, :constraints, args)}
end
end
135 changes: 135 additions & 0 deletions lib/phases/apply_constraints.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
defmodule AbsintheHelpers.Phases.ApplyConstraints do
@moduledoc """
Validates input nodes against constraints defined by the `constraints`
directive in your Absinthe schema. Constraints can be applied to fields
and arguments, enforcing rules such as `min`, `max`, etc. These constraints
can be applied to both individual items and lists simultaneously.

## Example usage

Add this phase to your pipeline in your router:

pipeline =
config
|> Absinthe.Plug.default_pipeline(opts)
|> AbsintheHelpers.Phases.ApplyConstraints.add_to_pipeline(opts)
Comment on lines +14 to +15

Choose a reason for hiding this comment

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

I do wonder if this is needed at all. They way I understand directives, they can (should) already include the implementation (with the extend block?).

I think it would simplify a little bit the introduction into projects.

Copy link
Member Author

@twist900 twist900 Sep 11, 2024

Choose a reason for hiding this comment

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

The directive is extended, but we still need to hook into the pipeline to interpret and execute the added constraints and their params

Copy link
Member Author

@twist900 twist900 Sep 11, 2024

Choose a reason for hiding this comment

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

Updated README here too

Choose a reason for hiding this comment

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

👌 I see it now.


Add the constraints directive's prototype schema to your schema:

defmodule MyApp.Schema do
use Absinthe.Schema
@prototype_schema AbsintheHelpers.Directives.Constraints
# ...
end

Apply constraints to a field or argument:

field :my_field, :integer, directives: [constraints: [min: 1, max: 10]] do
resolve(&MyResolver.resolve/3)
end

arg :my_arg, non_null(:string), directives: [constraints: [min: 10]]

field :my_list, list_of(:integer), directives: [constraints: [min_items: 2, max_items: 5, min: 1, max: 100]] do
resolve(&MyResolver.resolve/3)
end

Choose a reason for hiding this comment

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

Since you are already using field with body, maybe this form would be more readable:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I've now updated docs and test scenarios for demonstration of this 👍 ✅

Copy link
Member Author

Choose a reason for hiding this comment

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

btw, note how it is still useful to have directive inline as a key when it is applied to an arg and there is several of these args received by the field

Choose a reason for hiding this comment

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

yep

"""

use Absinthe.Phase

alias Absinthe.Blueprint
alias Absinthe.Phase
alias Blueprint.Input

def add_to_pipeline(pipeline, opts) do
Absinthe.Pipeline.insert_before(
pipeline,
Phase.Document.Validation.Result,
{__MODULE__, opts}
)
end

@impl Absinthe.Phase
def run(input, _opts \\ []) do
{:ok, Blueprint.postwalk(input, &handle_node/1)}
end

defp handle_node(
node = %{
input_value: %{normalized: normalized},
schema_node: %{__private__: private}
}
) do
if constraints?(private), do: apply_constraints(node, normalized), else: node
end

defp handle_node(node), do: node

defp apply_constraints(node, list = %Input.List{items: _items}) do
with {:ok, _list} <- validate_list(list, node.schema_node.__private__),
{:ok, _items} <- validate_items(list.items, node.schema_node.__private__) do
node
else
{:error, reason, details} -> add_custom_error(node, reason, details)
end
end

defp apply_constraints(node, %{value: _value}) do
case validate_item(node.input_value, node.schema_node.__private__) do
{:ok, _validated_value} -> node
{:error, reason, details} -> add_custom_error(node, reason, details)
end
end

defp apply_constraints(node, _), do: node

defp validate_list(list, private_tags) do
apply_constraints_in_sequence(list, get_constraints(private_tags))
end

defp validate_items(items, private_tags) do
Enum.reduce_while(items, {:ok, []}, fn item, {:ok, acc} ->
case validate_item(item, private_tags) do
{:ok, validated_item} -> {:cont, {:ok, acc ++ [validated_item]}}
{:error, reason, details} -> {:halt, {:error, reason, details}}
end
end)
end

defp validate_item(item, private_tags) do
apply_constraints_in_sequence(item, get_constraints(private_tags))
end

defp apply_constraints_in_sequence(item, constraints) do
Enum.reduce_while(constraints, {:ok, item}, fn constraint, {:ok, acc} ->
case call_constraint(constraint, acc) do
{:ok, result} -> {:cont, {:ok, result}}
{:error, reason, details} -> {:halt, {:error, reason, details}}
end
end)
end

defp call_constraint(constraint = {name, _args}, input) do
get_constraint_module(name).call(input, constraint)
end

defp get_constraint_module(constraint_name) do
String.to_existing_atom(
"Elixir.AbsintheHelpers.Constraints.#{Macro.camelize(Atom.to_string(constraint_name))}"
Copy link
Member Author

@twist900 twist900 Sep 10, 2024

Choose a reason for hiding this comment

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

note that unlike transforms, here we can't pass constraint modules directly as whatever we pass to a directive is rendered in the graphql schema itself 😓

Choose a reason for hiding this comment

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

🤔

well.. I'm still not fan of "guessing modules". But at this point it's just an implementation detail - it will work either way.

One thing I would suggest is dropping the Constaints modules. Right not they are mostly boilerplate, and logic form there could easily fit in this one - where all of them are documented. Something to think about

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we can keep the modules separate for now, lets see how this grows, its a reversible decision :)

)
end

defp get_constraints(private), do: Keyword.get(private, :constraints, [])

defp constraints?(private), do: private |> get_constraints() |> Enum.any?()

defp add_custom_error(node, reason, details) do
Phase.put_error(node, %Phase.Error{
phase: __MODULE__,
message: reason,
extra: %{
details: Map.merge(details, %{field: node.name})

Choose a reason for hiding this comment

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

👌

that much better than what we got.

I would be much better if we could provide full path to it (like create_booing.service.cost), but I'm not sure if that's possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be much better if we could provide full path to it (like create_booing.service.cost), but I'm not sure if that's possible.

yep, in a separate PR 👍

}
})
end
end
6 changes: 3 additions & 3 deletions lib/phases/apply_transforms.ex
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ defmodule AbsintheHelpers.Phases.ApplyTransforms do
`AbsintheHelpers.Transforms.ToInteger`, or within your own project, as long
as they implement the same behaviour.

## Example Usage
## Example usage

To add this phase to your pipeline, add the following to your router:

Expand Down Expand Up @@ -73,10 +73,10 @@ defmodule AbsintheHelpers.Phases.ApplyTransforms do
end

defp handle_node(
%{
node = %{
input_value: %{normalized: normalized},
schema_node: %{__private__: private}
} = node
}
) do
if transform?(private), do: apply_transforms(node, normalized), else: node
end
Expand Down
5 changes: 3 additions & 2 deletions mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule AbsintheHelpers.MixProject do
def project do
[
app: :absinthe_helpers,
version: "0.1.3",
version: "0.1.4",
elixir: "~> 1.16",
start_permanent: Mix.env() == :prod,
deps: deps(),
Expand Down Expand Up @@ -32,7 +32,8 @@ defmodule AbsintheHelpers.MixProject do
{:dialyxir, "~> 1.0", only: :dev, runtime: false},
{:ex_doc, "~> 0.34", only: :dev, runtime: false},
{:absinthe, "~> 1.0"},
{:mimic, "~> 1.10", only: :test}
{:mimic, "~> 1.10", only: :test},
{:decimal, "~> 1.9"}
]
end

Expand Down
1 change: 1 addition & 0 deletions mix.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"absinthe": {:hex, :absinthe, "1.7.8", "43443d12ad2b4fcce60e257ac71caf3081f3d5c4ddd5eac63a02628bcaf5b556", [:mix], [{:dataloader, "~> 1.0.0 or ~> 2.0", [hex: :dataloader, repo: "hexpm", optional: true]}, {:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}, {:nimble_parsec, "~> 1.2.2 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}, {:opentelemetry_process_propagator, "~> 0.2.1 or ~> 0.3", [hex: :opentelemetry_process_propagator, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "c4085df201892a498384f997649aedb37a4ce8a726c170d5b5617ed3bf45d40b"},
"bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"},
"credo": {:hex, :credo, "1.7.7", "771445037228f763f9b2afd612b6aa2fd8e28432a95dbbc60d8e03ce71ba4446", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "8bc87496c9aaacdc3f90f01b7b0582467b69b4bd2441fe8aae3109d843cc2f2e"},
"decimal": {:hex, :decimal, "1.9.0", "83e8daf59631d632b171faabafb4a9f4242c514b0a06ba3df493951c08f64d07", [:mix], [], "hexpm", "b1f2343568eed6928f3e751cf2dffde95bfaa19dd95d09e8a9ea92ccfd6f7d85"},
"dialyxir": {:hex, :dialyxir, "1.4.3", "edd0124f358f0b9e95bfe53a9fcf806d615d8f838e2202a9f430d59566b6b53b", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "bf2cfb75cd5c5006bec30141b131663299c661a864ec7fbbc72dfa557487a986"},
"earmark_parser": {:hex, :earmark_parser, "1.4.41", "ab34711c9dc6212dda44fcd20ecb87ac3f3fce6f0ca2f28d4a00e4154f8cd599", [:mix], [], "hexpm", "a81a04c7e34b6617c2792e291b5a2e57ab316365c2644ddc553bb9ed863ebefa"},
"erlex": {:hex, :erlex, "0.2.7", "810e8725f96ab74d17aac676e748627a07bc87eb950d2b83acd29dc047a30595", [:mix], [], "hexpm", "3ed95f79d1a844c3f6bf0cea61e0d5612a42ce56da9c03f01df538685365efb0"},
Expand Down
Loading