feat: Add validation queries (WIP)

[sezaru]

This is a first version of having validation queries for mutations. The idea is to have an API that will work similarly to what AshPhoenix.Form.validate does, in other words, instead of having to duplicate validation logic in the frontend, the frontend can call the validation query and get the data directly from the backend.

Right now there are some issues with this PR:

1. The validation queries are generated as mutations but they don't mutate anything, so maybe we should change them to be queries?
   One possible argument against this is that this would make using the validation api harder for the frontend developer since they would need to have a specific query for a the validation query and one for the mutation, while if both are mutations, then the only change is the function name.

Also, another thing to think about is that maybe we could instead of generating a new graphql api only to validate, we could add a validate input to the mutation itself and then the frontend keeps using the same API for both cases, just changing the validate boolean to define when validate and when submit (honestly, now that I think about it, I think this would be probably the best solution both in our side and also for implementing in the frontend).

2. I implemented it for create and update actions, I didn't do it for generic actions, maybe we should have that too?

3. I'm not sure if I should add support for all the other options that create and update actions have in their schema to the validation actions.

4. I didn't add any unit tests for now since this has a great chance to change.

Here is a resource that I used to test it out:

defmodule ValidateGraphql.Domain.Resource do
  @moduledoc false

  use Ash.Resource,
      domain: ValidateGraphql.Domain,
      data_layer: AshPostgres.DataLayer,
      extensions: [AshGraphql.Resource]

  attributes do
    uuid_primary_key :id

    attribute :first_attr, :string, public?: true
    attribute :second_attr, :string, public?: true, allow_nil?: false
  end

  graphql do
    type :resource

    mutations do
      create :create, :create
      validate :validate_create, :create

      update :update, :update
      validate :validate_update, :update
    end
  end

  postgres do
    table "resource"

    repo ValidateGraphql.Repo
  end

  identities do
    identity :unique_second_attr, [:second_attr]
  end

  actions do
    defaults [:read, :destroy]

    create :create do
      accept [:first_attr, :second_attr]

      argument :first_arg, :string
      argument :second_arg, :string, allow_nil?: false
    end

    update :update do
      accept [:first_attr, :second_attr]

      argument :first_arg, :string
      argument :second_arg, :string, allow_nil?: false
    end
  end
end

Contributor checklist

• Features include unit/acceptance tests (it doesn't for now)

[sezaru]

Now that I think about it, ignore the part about adding the validate option to the mutation directly, even though I believe that would be the best approach for ease of use, it would not work since the validate version of the mutation needs to not have any field as required.

[zachdaniel]

👋 just want to let you know this is still on my list. I've been mega busy the past week or so. I hope to get to it this week but it may be next week before I have the chance.

[sezaru]

Don't worry, take your time!

[zachdaniel]

So your idea of making validate an option to mutations is actually really interesting. I don't think we can do it for default queries because they don't return a "result object", but theoretically if there is an input called something like onlyValidate: true, then we don't need any fancy new type generation or anything! We can just return an empty list of errors. We have to keep in mind that validating will just do the "cheap" validations not necessarily everything so you can of course still get errors back, but I think that idea is a winner and I'd like to see that explored. Would you be willing to spike out that work?

[sezaru]

The issue that I see with that is regarding non-nilable fields, for example, with the current code, the generated validate apis will change all non-nilable fields to allow nil to bypass the Absinthe check and reach the Ash.Changeset validation.

If we only have the option, then I think we would need a way to bypass that check in Absinthe, otherwise the api will just return one error (for the non-nilable field not being passed) instead of returning all current errors which is what we should expect from a validation.

I did take a look into that a week ago, there is a phase that will do these checks in Absinthe side (these two: https://github.com/absinthe-graphql/absinthe/blob/main/lib/absinthe/phase/document/validation/provided_non_null_arguments.ex and https://github.com/absinthe-graphql/absinthe/blob/main/lib/absinthe/phase/document/validation/provided_non_null_variables.ex), and that comes as default when we generate the Absinthe document object (https://github.com/absinthe-graphql/absinthe/blob/f88c226104d0e4def6d50eaa527ad70508c9a1cd/lib/absinthe/pipeline.ex#L52).

The issue I came by is that at the time we generate the pipeline (I tried to find here where that happens, buy I couldn't find it), if I remember correctly, we don't yet know if we should have these validations in place.

The correct approach would be to create the pipeline without these validations, and somehow add them dynamically depending on the onlyValidate field value.

If I would guess, I think we would need to actually bake our own version of these validations (that would just call the original behind the scenes) so we have access to the inputs and conditionally do the validation or not.

If you are ok with this idea, then I can try create these validations here and see if I can make it work. The only think I would need help is to understand where exactly in the code the pipeline is being created so I can remove the original validations from the pipeline and insert our custom ones.

[sezaru]

I do see this line (

ash_graphql/lib/ash_graphql.ex

Line 187 in 5ce7f6f

def pipeline(pipeline) do

) where we manipulate the pipeline in AshGraphql, to be honest I don't get who calls this function and how the pipeline is created and send as an argument to this function, but if this is guaranteed to always run when creating the pipeline, then I can use it to call Pipeline.replace(pipeline, ProvidedNonNullVariables, MyOwnMaybeProvidedNonNullVariables)

[zachdaniel]

Ah, right. Well... is it really a requirement that we allow making a validation request for a request that is literally impossible? The error messages produced would be impossible for the client to actually receive in practice. You'd be dealing with an impossible error state. That doesn't really seem like a good idea to me.

[sezaru]

I don't think I understood what you mean in your last message.

But here is breakdown of what I mean by changing the validation phase:

Right now, with the current code, if you don't fill the arguments that are required, you will get errors like this:

{
  "errors": [
    {
      "message": "In argument \"refCode\": Expected type \"String!\", found null.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
    },
    {
      "message": "In argument \"id\": Expected type \"ID!\", found null.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
    }
  ]
}

These errors are generated by that validation phase I mentioned before.

The issue with leaving the behavior for the validation option is that this phase will prevent the pipeline to reach the part that actually validates the Ash.Changeset.

For example, let's say that another issue from my mutation is that I also have an e-mail argument that has an invalid e-mail as value.

Since the pipeline stopped at the required arguments validation, it will never generate the error for the invalid e-mail field, even if the user already filled that field in the form.

This means that the output the api will give depends if all required fields are filled or not, which is not consistent with the way the validation works using a phoenix form for example.

What I'm proposing is to replace these phase validations with one that checks for the onlyValidate input to see if it should run or not, if onlyValidate is true, then the phase will not run and, consequently, the code will reach the part that it generates the Ash.Changeset and do the validation from it.

In the above code, the changeset would generate 3 errors, one for each field that is required but it is not provided (id and refCode) and also another one for the invalid email the user typed, giving the exact same behavior as the one we get from Phoenix forms right now.

[sezaru]

Oh, after re-reading you message I think I got what you meant by impossible state, but I don't agree with it.

So, let's think first what is the purpose of this PR:

The idea is to give the frontend a way to validate a form (that uses the graphql api behind the scenes) using the backend instead of duplicating code in the frontend.

By impossible state, I think you meant that the frontend would already validate the graphql api schema anyway and show that the required input fields are required without having the call the backend to do that (basically the input will check the api schema, see that its field is non-nilable and show an error if it is not filled yet).

The problem with this is that now the frontend user needs to have two implementations, one where he checks the graphql schema to make sure that the input is required or not and another one when he call the validation action and gets other errors from the backend.

I don't think is a good solution, a better solution IMO is that all the validations should be done by the backend, making the frontend code simpler and without code duplication.

That means that the frontend will not check if the field is mandatory or not, it will just call the api either way and just check the errors that it will return.

By doing that, and bypassing the phase validations I mentioned before, the frontend will get the full changeset errors and then they can inform the user of which validations have problems (the exact same way we do now with LiveView forms).

[zachdaniel]

Hm...yeah I guess that's true. And since it's opt-in, the user can test and ensure that their action behaves fine with no required inputs present. It's important to keep in mind that action changes still run even on invalid inputs, so the validate endpoint may require users to test it out and ensure their validations/changes don't have issues etc.

[zachdaniel]

I don't think we should bypass the phase validations though if we're doing that, it should be a separate mutation at that point

[sezaru]

In that case we would keep the current approach from the PR?

In the current code I keep these validation phases, but I change the required fields to not be required anymore in the graphql schema. In the end of the day they do the same thing. So I guess I could change to the above approach in the validation mutation just so the schema is the same?

Also, just one thing to be clear, the above suggestion would not disable these phases if the user are not doing a validation. Basically they would run if onlyValidate is false (the default), and not run if it is true). In other words, the behavior would be exactly the same it is now if the field onlyValidate is false.

[zachdaniel]

It's hard to decide 😂

so the issue I have with disabling the pipeline validations is that many implementations of GraphQL clients are typed and so will produce type system level errors trying to call the mutation with null inputs.

if we say "that doesn't matter they can fix it on their end", then our lives get a lot simpler and we can go with the approach of reusing the mutation.

but for some users I suspect that will be a deal breaker.

Perhaps we can start with that approach and make it extensible later, like validate_only: {:mutation, :new_mutation_name} | :in_place and only implement :in_place for now? That way we can wait until someone needs the more complex version of this as a separate mutation before adding it?

what do you think?

[sezaru]

Ah, I get what you mean, I'm always on the side of making the graphql apis, not consuming them, so I'm not super familiar with the existing libraries for it.

So, basically, there will be libraries that will block or warn the user if they are ignoring required arguments.

In that case, I think the best approach is just continue with our current one, create a validation version of the same API and make all required fields in it not required.

That way it will work for everyone, and it will not affect the existing apis in any way.

What do you think?

[zachdaniel]

Yep, so we should just continue on with this PR.

A few notes from my initial review:

1. We will likely need to support most of the options to each given mutation type. Looking over the implementation, I think the best way to do this is actually to just make it a flag to the existing mutation/query builders. I think perhaps you suggested something like this before, maybe I shot it down 😆
2. This way, instead of separate resolvers they switch on mutation/query.validate? is true. Then, at the end just before we would run the action, we instead just generate any errors and return those. This will fix the "code explosion" problem and also keep the existing tests very relevant.
3. This also lets them remain as their primary query and mutation counterparts. It makes sense for query validations to be queries and mutation validations to be mutations IMO.

[sezaru]

Just to double check if I got you comments right:

1. Yes, I removed them at the time because I was not sure which one should I support or not (because maybe some of them are only relevant when actually creating/updating the resource), if you don't mind, can you tell me which ones should I keep?
2. I'm not sure I understood this one fully, is the idea here to keep the validate option to create validation apis like it is now in the PR? The flag you mention is just an internal flag to avoid duplicating so much code during the api generation?

Also, should we also support generic actions (right now it just supports create and update actions)? You also mentioned queries, but I'm not sure show exactly we would validate a query

[zachdaniel]

I'm not sure I understood this one fully, is the idea here to keep the validate option to create validation apis like it is now in the PR? The flag you mention is just an internal flag to avoid duplicating so much code during the api generation?

What I mean is that we should do something like

create :validate_create, :create, validate_only?: true

So that it goes down the complete same codepath right up until the end.

[zachdaniel]

You can validate ActionInput and Querys as well, ultimately they are built in the same way as changesets and can contain errors in the same way as well.