validate field arguments #608

crissi · 2020-03-21T22:35:52Z

Summary

Fixes #602

Rules never worked in field arguments.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

Existing tests have been adapted and/or new tests have been added
Add a CHANGELOG.md entry
Code style has been fixed via composer fix-style

crissi · 2020-03-21T22:38:01Z

Depth is hardcoded, would adding a static setter be sufficient?

ResolveInfoFieldsAndArguments is fired twice if you are also using SelectField, maybe this could be moved, out, any suggestion to how you would do that @mfn ?

mfn · 2020-03-26T21:23:01Z

@crissi ready for review?

crissi · 2020-03-26T21:49:07Z

yes

mfn

Depth is hardcoded, would adding a static setter be sufficient?

ResolveInfoFieldsAndArguments is fired twice if you are also using SelectField, maybe this could be moved, out, any suggestion to how you would do that @mfn ?

Thanks for pointing this out.

IMHO yes, we definitely should improve this; ResolveInfoFieldsAndArguments can be quite costly.

Since validation is an "always on" feature, we have to resolve this anyway so we can do it already before calling validateFieldArguments and keep a reference to the result, which we then also pass to instanciateSelectFields() and we can already pass it directly into \Rebing\GraphQL\Support\SelectFields::__construct, removing/refactoring \Rebing\GraphQL\Support\SelectFields::getFieldSelection

Regarding the depth: maybe we need to change this approach:

we keep the default of 5 hardcoded
but add a config to override that default
and add the ability to define the depth optionally on the query => would that be possible (just as config option on the graphql query type?)

Does that make sense?

I might have more feedback but we should sort out the high-level stuff first, thanks!

src/Support/RulesInFields.php

mfn

I think this looks great!

A few minor feedbacks but after that and update with master it's good to merge from my side, thank you!

mfn · 2020-04-19T18:02:09Z

src/Support/Field.php

@@ -123,6 +145,11 @@ protected function getResolver(): ?Closure
                }
            }

+            $fieldsAndArguments = (new ResolveInfoFieldsAndArguments($arguments[3]))->getFieldsAndArgumentsSelection($this->depth);


No the big prize question: how can we avoid resolving this if there aren't going to be any validations (they're optional after all)? Do you see any way to achieve this?

Seems like a catch-22: only when we run RulesInFields for which we already have to extract "fields and arguments" we can then know that we wouldn't have to run it 😄

I could probably write a function that checks if any arguments exists that would be less costly than the ResolveInfoFieldsAndArguments, and then only run ResolveInfoFieldsAndArguments if using selectfields and arguments... but not sure how much it will actually give.

You're right, let's scrap that for now!

CHANGELOG.md

src/Support/Field.php

src/Support/RulesInFields.php

src/Support/SelectFields.php

mfn · 2020-04-20T21:04:01Z

The job with the integration test is failing, I'll restart it

mfn · 2020-04-20T21:12:12Z

hm, nope, fails too https://travis-ci.org/github/rebing/graphql-laravel/jobs/677399766#L380 :

PHP Fatal error:  Allowed memory size of 1610612736 bytes exhausted (tried to allocate 4096 bytes) in phar:///home/travis/.phpenv/versions/7.3.17/bin/composer/src/Composer/DependencyResolver/RuleWatchGraph.php on line 52

That's 1.6GB so no point in bumping the memory_limit, something is off here.

My first idea maybe something in composer changed, a newer version, but https://travis-ci.org/github/rebing/graphql-laravel/jobs/677399766#L148

Composer version 1.8.4 2019-02-11 10:52:10

I pushed a commit to see how it goes when we composer self-update…

The new 2.0 as of now

mfn · 2020-04-20T21:19:17Z

https://travis-ci.org/github/rebing/graphql-laravel/jobs/677442929#L152

Composer version 1.10.5 2020-04-10 11:44:22

$ composer self-update
You are already using composer version 1.10.5 (stable channel).

Ach #fail

Meh, must be something else as the trace comes from within the resolver…

I'll try the composer 2.0 preview, maybe we're lucky :)

mfn · 2020-04-20T21:23:16Z

I'll try the composer 2.0 preview, maybe we're lucky :)

We were 🍀 !

This reverts commit 81ed042

mfn · 2020-04-23T09:37:56Z

@crissi you might already noticed, this added a regression see #627

Please feel free to "revert my revert" and apply a fix (probably the InvariantException thingy, 95% sure)

Thanks!

(and sorry for my feedback: pretty sure I was the reason why we removed it)

crissi · 2020-04-23T09:39:44Z

Np, will make a fix later Hent Outlook til iOS<https://aka.ms/o0ukef>

…

________________________________ Fra: Markus Podar <notifications@github.com> Sendt: Thursday, April 23, 2020 11:38:11 AM Til: rebing/graphql-laravel <graphql-laravel@noreply.github.com> Cc: Christian <c2n@live.dk>; Mention <mention@noreply.github.com> Emne: Re: [rebing/graphql-laravel] validate field arguments (#608) @crissi<https://github.com/crissi> you might already noticed, this added a regression see #627<#627> Please feel free to "revert my revert" and apply a fix (probably the InvariantException thingy, 95% sure) Thanks! (and sorry for my feedback: pretty sure I was the reason why we removed it) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#608 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAXPDYQ57XSNUKHL7HZVKZDROAEAHANCNFSM4LRCJBEQ>.

This reverts commit fb9dace.

crissi changed the title ~~[WIP] validate field arguments~~ validate field arguments Mar 26, 2020

mfn requested changes Apr 3, 2020

View reviewed changes

mfn approved these changes Apr 19, 2020

View reviewed changes

crissi added 9 commits April 20, 2020 20:29

validate field arguments

36f6c6e

fixes

2392bab

fix

17f7194

fixes

9984377

fix

f96922e

update readme

00b922d

refactor

689a115

fix style

a63ed17

fixes

427d6d9

crissi force-pushed the add_subquery_args_validations branch from afdaa6e to 427d6d9 Compare April 20, 2020 18:48

fix

cf8c536

travis: update composer

b91e7df

travis: update composer to the latest snapshot

82438f7

The new 2.0 as of now

mfn merged commit 81ed042 into rebing:master Apr 20, 2020

mfn mentioned this pull request Apr 23, 2020

__typename is not defined. Bug in 5.1 #627

Closed

mfn added a commit that referenced this pull request Apr 23, 2020

Revert "validate field arguments (#608)"

fb9dace

This reverts commit 81ed042

mfn mentioned this pull request Apr 23, 2020

[HOTFIX] Prepare 5.1.1 release and revert "Add support for validation in field arguments" #628

Merged

9 tasks

crissi pushed a commit to crissi/graphql-laravel that referenced this pull request Jun 8, 2020

Revert "Revert "validate field arguments (rebing#608)""

a72e6a1

This reverts commit fb9dace.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate field arguments #608

validate field arguments #608

crissi commented Mar 21, 2020 •

edited

Loading

crissi commented Mar 21, 2020

mfn commented Mar 26, 2020

crissi commented Mar 26, 2020

mfn left a comment

mfn left a comment

mfn Apr 19, 2020

crissi Apr 20, 2020

mfn Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 23, 2020

crissi commented Apr 23, 2020 via email

validate field arguments #608

validate field arguments #608

Conversation

crissi commented Mar 21, 2020 • edited Loading

Summary

Type of change

Checklist

crissi commented Mar 21, 2020

mfn commented Mar 26, 2020

crissi commented Mar 26, 2020

mfn left a comment

Choose a reason for hiding this comment

mfn left a comment

Choose a reason for hiding this comment

mfn Apr 19, 2020

Choose a reason for hiding this comment

crissi Apr 20, 2020

Choose a reason for hiding this comment

mfn Apr 20, 2020

Choose a reason for hiding this comment

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 20, 2020

mfn commented Apr 23, 2020

crissi commented Apr 23, 2020 via email

crissi commented Mar 21, 2020 •

edited

Loading