Add constraints phase #4

twist900 · 2024-09-09T18:10:26Z

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.

⚠️ ⚠️ ⚠️ Note that constraints are intentionally implemented using GraphQL directives, which means that they will be rendered on the graphql schema itself and visible to the FE. For example:

"Overrides for location-specific service pricing."
input LocationOverrideInput {
  duration: Int @constraints(min: 300, max: 43200)
  price: Decimal @constraints(min: 0, max: 100000000)
  priceType: ServicePriceType
  locationId: ID!
}

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)

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_list, list_of(:integer) do
    directive(:constraints, [min_items: 2, max_items: 5, min: 1, max: 100])

    resolve(&MyResolver.resolve/3)
  end

  field :my_field, :integer do
    arg :my_arg, non_null(:string), directives: [constraints: [min: 10]]

    resolve(&MyResolver.resolve/3)
  end

twist900 · 2024-09-10T11:48:05Z

lib/phases/apply_constraints.ex

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


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 😓

🤔

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

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

bikashpf · 2024-09-10T17:37:18Z

lib/constraints/max.ex

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


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?

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

btw, here how it look all together on a real-life schema:

https://github.com/surgeventures/system/blob/e321afcf26677b0dc40b86d5a37f740422ef0d81/apps/shedul-umbrella/apps/partners_api_gql/lib/partners_api_gql/schema/catalog.ex

bikashpf · 2024-09-10T17:37:55Z

test/absinthe_helpers/phases/apply_constraints_test.exs

+                  %{message: :min_not_met, details: %{field: "override_ids", min: 5, value: 1}},
+                  %{
+                    message: :min_items_not_met,
+                    details: %{field: "location_ids", min_items: 2, items: [1]}


Looks like it's missing scenario for max_items_not_met.

now done here ✅

mpmiszczyk · 2024-09-11T06:54:48Z

lib/phases/apply_constraints.ex

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


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

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

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

mpmiszczyk · 2024-09-11T07:47:49Z

lib/phases/apply_constraints.ex

+        |> Absinthe.Plug.default_pipeline(opts)
+        |> AbsintheHelpers.Phases.ApplyConstraints.add_to_pipeline(opts)


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.

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

Updated README here too

👌 I see it now.

mpmiszczyk

Please do review (rethink? is that right word?) values being returned in errors. I'm still 👌 with discussing it, just would like to be sure that it will be considered.

And the valuse is the only real "issue" I have.

mpmiszczyk · 2024-09-11T16:49:11Z

README.md

@@ -1,16 +1,132 @@
-# AbsintheHelpers
+# Absinthe Helpers


Nice.

Since you are using README.md you can use it at a main page of the docs - like we are doing here

mpmiszczyk · 2024-09-11T17:00:55Z

lib/phases/apply_constraints.ex

+      phase: __MODULE__,
+      message: reason,
+      extra: %{
+        details: Map.merge(details, %{field: node.name})


👌

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.

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 👍

mpmiszczyk · 2024-09-11T17:04:37Z

lib/constraints/max.ex

+    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}


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 :/

(and that goes for all values added to errors)

ok, removed user values from errors ✅

mpmiszczyk · 2024-09-11T17:26:28Z

lib/phases/apply_constraints.ex

+        |> Absinthe.Plug.default_pipeline(opts)
+        |> AbsintheHelpers.Phases.ApplyConstraints.add_to_pipeline(opts)


👌 I see it now.

mpmiszczyk · 2024-09-11T17:45:47Z

lib/phases/apply_constraints.ex

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


🤔

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

mpmiszczyk · 2024-09-11T17:51:39Z

lib/constraints/max.ex

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


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

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

twist900 force-pushed the add-constraints-phase branch from 92d473e to 781ea8f Compare September 9, 2024 18:16

Base automatically changed from add-transformations-phase to main September 10, 2024 09:36

twist900 force-pushed the add-constraints-phase branch from 781ea8f to 8ad276d Compare September 10, 2024 11:23

twist900 self-assigned this Sep 10, 2024

twist900 force-pushed the add-constraints-phase branch 3 times, most recently from cc66e90 to 1e9ed35 Compare September 10, 2024 11:44

twist900 commented Sep 10, 2024

View reviewed changes

twist900 requested review from kamilkowalski, mpmiszczyk, perzanko, bikashpf, dsharma29 and swarupsengupta September 10, 2024 11:50

twist900 force-pushed the add-constraints-phase branch from 1b66424 to c86b0d7 Compare September 10, 2024 14:22

twist900 added 2 commits September 10, 2024 15:23

Add constraints phase

105a56a

Bump version

8361bf0

twist900 force-pushed the add-constraints-phase branch from c86b0d7 to 8361bf0 Compare September 10, 2024 14:24

bikashpf reviewed Sep 10, 2024

View reviewed changes

Add test for max_items_exceeded

dde1a1d

mpmiszczyk reviewed Sep 11, 2024

View reviewed changes

Improve apply_constraint documentation

ce15186

twist900 force-pushed the add-constraints-phase branch from 610fdd7 to 509e123 Compare September 11, 2024 08:51

Improve README

6641e6e

twist900 force-pushed the add-constraints-phase branch from 509e123 to 6641e6e Compare September 11, 2024 08:54

mpmiszczyk requested changes Sep 11, 2024

View reviewed changes

mpmiszczyk reviewed Sep 11, 2024

View reviewed changes

Add readme as docs

e6fb457

Remove value from error details

758d20d

twist900 requested a review from mpmiszczyk September 11, 2024 21:17

mpmiszczyk approved these changes Sep 12, 2024

View reviewed changes

twist900 merged commit 9bce778 into main Sep 12, 2024
4 checks passed

twist900 deleted the add-constraints-phase branch September 12, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constraints phase #4

Add constraints phase #4

twist900 commented Sep 9, 2024 •

edited

Loading

twist900 Sep 10, 2024 •

edited

Loading

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

bikashpf Sep 10, 2024

twist900 Sep 10, 2024

twist900 Sep 10, 2024

bikashpf Sep 10, 2024

twist900 Sep 10, 2024

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

twist900 Sep 11, 2024

mpmiszczyk Sep 11, 2024

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024 •

edited

Loading

twist900 Sep 11, 2024 •

edited

Loading

mpmiszczyk Sep 11, 2024

mpmiszczyk left a comment •

edited

Loading

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

mpmiszczyk Sep 11, 2024

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

mpmiszczyk Sep 11, 2024

mpmiszczyk Sep 11, 2024

mpmiszczyk Sep 11, 2024

twist900 Sep 11, 2024

		\|> Absinthe.Plug.default_pipeline(opts)
		\|> AbsintheHelpers.Phases.ApplyConstraints.add_to_pipeline(opts)

Add constraints phase #4

Add constraints phase #4

Conversation

twist900 commented Sep 9, 2024 • edited Loading

Example usage

twist900 Sep 10, 2024 • edited Loading

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

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

twist900 Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

twist900 Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpmiszczyk left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twist900 commented Sep 9, 2024 •

edited

Loading

twist900 Sep 10, 2024 •

edited

Loading

twist900 Sep 11, 2024 •

edited

Loading

twist900 Sep 11, 2024 •

edited

Loading

mpmiszczyk left a comment •

edited

Loading