Do not allow users to apply fold, recurse and transform directives to properties #457

era · 2023-08-21T16:41:12Z

Validates on the frontend if the query applies @fold, @recurse or @transform on a property returning a new error (UnsupportedDirectiveOnProperty) if so.

trustfall_core/test_data/tests/frontend_errors/transform_used_on_property.frontend-error.ron

obi1kenobi

Nice work! Uncovered a bit of a design question in the process, which I'm sure should be possible to overcome without too much trouble.

obi1kenobi · 2023-08-22T02:45:33Z

trustfall_core/src/frontend/error.rs

+    #[error(
+        "The field \"{0}\" is a property but the query uses @fold.\
+     Only edges can be used with @fold."
+    )]
+    FoldOnProperty(String),
+


A few questions here. I genuinely don't know the answer to these off the top of my head, but we probably want to consider them so as to make consistent and intuitive choices:

What error does the frontend raise for other edge-only directives (@optional / @recurse) is used on a property? (Not sure if we already have test cases for these, but we should add them if we don't!)

Would it make sense to use a more general variant for all such directives that are invalid on properties, like a UnsupportedDirectiveOnProperty? (Name subject to workshopping, open to ideas.)

Also, a couple of nits:

We are not guaranteeing enum variant order, so let's try to group this variant near other variants for related errors instead.

The error message is a bit unclear: for example, it only says that @fold was used by never says the actual problem — that it was used by applying it to a property. The existing error messages here are also not good, but good diagnostics are super important so we should try to start raising the quality bar if at all possible.

What error does the frontend raise for other edge-only directives (@optional / @recurse) is used on a property? (Not sure if we already have test cases for these, but we should add them if we don't!)

Unless the playground is outdated and validations were added after, those do not raise any errors. So it feels like this change should include those directives as well and use naming like you proposed (UnsupportedDirectiveOnProperty)

We are not guaranteeing enum variant order, so let's try to group this variant near other variants for related errors instead.
The error message is a bit unclear: for example, it only says that @fold was used by never says the actual problem — that it was used by applying it to a property. The existing error messages here are also not good, but good diagnostics are super important so we should try to start raising the quality bar if at all possible.

I will send out a version with a little better wording, but I will sleep on it to see if I can improve it.

I don't think any validations have been added in a while. Oops! Great catch, thanks for finding this!

trustfall_core/test_data/tests/frontend_errors/transform_used_on_property.frontend-error.ron

trustfall_core/src/frontend/mod.rs

trustfall_core/src/frontend/error.rs

era · 2023-08-24T07:34:41Z

trustfall_core/src/frontend/mod.rs

@@ -1104,6 +1103,30 @@ where
        {
            // Processing a property.

+            // @fold is not allowed on a property
+            if connection.fold.is_some() {


There is some code duplication around here, but at least the signatures I thought about would keep things highly coupled with the context of this method. So I kept the code duplication because I thought it was the cleaner version of the code. Let me know if you have ideas for making this better.

This is totally fine. Not all duplication is bad, and in this case the duplicated code is fairly short and very clear. Anything less duplicated would be way more convoluted and less readable.

obi1kenobi

This looks great, I'd be happy to merge as soon as the fix for that one test case is in 🎉

obi1kenobi · 2023-08-24T13:55:41Z

trustfall_core/src/frontend/mod.rs

@@ -1104,6 +1103,30 @@ where
        {
            // Processing a property.

+            // @fold is not allowed on a property
+            if connection.fold.is_some() {


This is totally fine. Not all duplication is bad, and in this case the duplicated code is fairly short and very clear. Anything less duplicated would be way more convoluted and less readable.

trustfall_core/test_data/tests/frontend_errors/recurse_used_on_property.graphql.ron

trustfall_core/src/frontend/error.rs

…_property.graphql.ron Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

obi1kenobi · 2023-08-24T14:02:55Z

Also remember to update the PR title and description to include the other directives being handled!

obi1kenobi · 2023-08-24T14:10:42Z

Ah the failing lints are because we got a new Rust stable today. I'll push a fix real quick, sorry about that.

era · 2023-08-24T14:24:15Z

Ah thanks for clarifying, I was almost rolling back the last changes to see if the problem would remain :D

obi1kenobi · 2023-08-24T14:39:17Z

Marked for merge as soon as CI approves! 🎉

Have you already picked out the next thing to tackle, or would you like some ideas for things to look at? Assuming you still have time to contribute, of course no pressure.

does not allow properties to use fold

41ae813

era commented Aug 21, 2023

View reviewed changes

trustfall_core/test_data/tests/frontend_errors/transform_used_on_property.frontend-error.ron Outdated Show resolved Hide resolved

era marked this pull request as ready for review August 21, 2023 16:52

era changed the title ~~Does not allow properties to use fold~~ Do not allow properties to use fold Aug 21, 2023

obi1kenobi reviewed Aug 22, 2023

View reviewed changes

era added 3 commits August 22, 2023 13:38

rename test

3611415

rename test

401b620

renaming error

bfe1a6c

era commented Aug 22, 2023

View reviewed changes

trustfall_core/src/frontend/mod.rs Outdated Show resolved Hide resolved

obi1kenobi reviewed Aug 23, 2023

View reviewed changes

trustfall_core/src/frontend/error.rs Outdated Show resolved Hide resolved

era added 5 commits August 24, 2023 08:16

move error

b8fedd4

move error

4c1fc22

@optional tests

f4377a4

adding recurse tests

ed9694e

uses is_some

2e6d6f0

era commented Aug 24, 2023

View reviewed changes

obi1kenobi reviewed Aug 24, 2023

View reviewed changes

era and others added 2 commits August 24, 2023 15:00

Update trustfall_core/test_data/tests/frontend_errors/recurse_used_on…

87d1ec7

…_property.graphql.ron Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Update trustfall_core/src/frontend/error.rs

1013a80

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

obi1kenobi added A-errors Area: external-facing error functionality R-relnotes Release: document this in the release notes of the next release labels Aug 24, 2023

fixing recurse test

d6e06d0

era changed the title ~~Do not allow properties to use fold~~ Do not allow properties to use fold, recurse and transform Aug 24, 2023

era changed the title ~~Do not allow properties to use fold, recurse and transform~~ Do not allow properties to use fold, recurse and transform directives Aug 24, 2023

era changed the title ~~Do not allow properties to use fold, recurse and transform directives~~ Do not allow users to apply fold, recurse and transform directives on properties Aug 24, 2023

era changed the title ~~Do not allow users to apply fold, recurse and transform directives on properties~~ Do not allow users to apply fold, recurse and transform directives to properties Aug 24, 2023

Merge branch 'main' into era/transform

39e9247

obi1kenobi approved these changes Aug 24, 2023

View reviewed changes

obi1kenobi enabled auto-merge (squash) August 24, 2023 14:37

obi1kenobi merged commit ef9790c into obi1kenobi:main Aug 24, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow users to apply fold, recurse and transform directives to properties #457

Do not allow users to apply fold, recurse and transform directives to properties #457

era commented Aug 21, 2023 •

edited

Loading

obi1kenobi left a comment

obi1kenobi Aug 22, 2023

era Aug 22, 2023

obi1kenobi Aug 22, 2023

era Aug 24, 2023

obi1kenobi Aug 24, 2023

obi1kenobi left a comment

obi1kenobi Aug 24, 2023

obi1kenobi commented Aug 24, 2023

obi1kenobi commented Aug 24, 2023

era commented Aug 24, 2023

obi1kenobi commented Aug 24, 2023

Do not allow users to apply fold, recurse and transform directives to properties #457

Do not allow users to apply fold, recurse and transform directives to properties #457

Conversation

era commented Aug 21, 2023 • edited Loading

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi Aug 22, 2023

Choose a reason for hiding this comment

era Aug 22, 2023

Choose a reason for hiding this comment

obi1kenobi Aug 22, 2023

Choose a reason for hiding this comment

era Aug 24, 2023

Choose a reason for hiding this comment

obi1kenobi Aug 24, 2023

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi Aug 24, 2023

Choose a reason for hiding this comment

obi1kenobi commented Aug 24, 2023

obi1kenobi commented Aug 24, 2023

era commented Aug 24, 2023

obi1kenobi commented Aug 24, 2023

era commented Aug 21, 2023 •

edited

Loading