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

Fix during migration defaults are not compiled as detached #5606

Merged
merged 1 commit into from Jun 12, 2023

Conversation

aljazerzen
Copy link
Contributor

@aljazerzen aljazerzen commented Jun 6, 2023

Closes #5547

@aljazerzen aljazerzen requested a review from msullivan June 6, 2023 15:53
Comment on lines 608 to 613
CREATE TYPE TestSelfLink3 {
CREATE PROPERTY foo3 -> std::str;
CREATE PROPERTY bar3 -> std::str {
SET default := TestSelfLink3.foo3;
SET default := .foo3;
};
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix broke this test.

Turns out if you tried to do an INSERT after this test, it would fail. So it was wrong before.

@@ -1838,7 +1844,7 @@ def _check_id_default(
'std::uuid_generate_v4',
)

if (
while (
isinstance(expr, irast.Set)
and expr.expr
and irutils.is_trivial_select(expr.expr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiling as detached produced double nested irast.Set

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Could you double check how other things that are detached handle this (policies, rewrites), since I don't remember, and we might want to unify the mechanisms?

We also need to have a more general discussion of what the plan is for schema things being detached, since the backstory here is kind of twisted and we're in a pretty mixed state right now

@aljazerzen
Copy link
Contributor Author

aljazerzen commented Jun 9, 2023

I've done an analysis of supported features and this is the resulting matrix:

.property User.property dunder
defaults yes (correlated) detached __source__ (bugged currently)
rewrites yes (correlated, __subject__) detached __subject__, __new__
triggers for each no detached __old__, __new__
triggers for all no detached __old__, __new__
policies yes (correlated, __subject__) detached __subject__
indexes yes (correlated, __subject__) correlated __subject__
constraints on pointers yes (correlated with the pointer) detached __subject__ (refers to the pointer)
constraints on obj types yes (correlated with the object) detached __subject__ (refers to the object)
computeds yes (correlated with the object) correlated __source__

All in all, pretty consistent. These are the things we could improve:

  • allow .property in triggers,
  • make User.property in computeds and indexes detached,
  • unify __subject__ and __source__ (maybe).

PS: these are all tested for properties only, no links or link properties.

Schema used
module default {

    type User1 {

        required property nick: str;
        
        required property name: str {

            default := (select .nick);

            rewrite insert using (select .nick);

            # Error: invalid property reference on a primitive type expression
            # -> it is desugared to User1.name.name
            # constraint expression on (.name like 'a%');
        };

        # trigger name_insert after insert for each do (
        #     assert_single((select .nick))
        # );

        # trigger name_insert_all after insert for all do (
        #     assert_single((select .nick))
        # );

        access policy hide_unnamed
            allow select
            using (select .nick != "");
        access policy write
            allow insert, update, delete;

        index on (.name);

        constraint exclusive on (.name);

        single property upper_name := str_upper(.name);
    }

    type User2 {

        required property nick: str;

        required property name: str {

            default := (select User2.nick);

            # Error: missing value for required property 'name'
            # -> non-correlated
            # rewrite insert using (select User2.nick);

            # Error: expression is defined recursively
            # -> uncorrelated
            # constraint expression on (User2.name like 'a%');
        };

        # Error after inserting the second User2 -> uncorrelated
        # trigger name_insert after insert for each do (
        #     assert_single((select User2.nick))
        # );

        # Error after inserting the second User2 -> uncorrelated
        trigger name_insert_all after insert for all do (
            assert_single((select User2.nick))
        );

        # Error: possibly empty set -> non-correlated
        # access policy hide_unnamed
        #     allow select
        #     using (User2.nick != "");

        # No error, which means that User2.name is single,
        # which means it's correlated.
        index on (User2.name);

        # Error: CardinalityViolationError after second insert,
        # even for different value of User2.name
        # -> it is comparing the whole set of User2.name against itself
        # -> uncorrelated
        constraint exclusive on (User2.name);

        single property upper_name := str_upper(User2.name);
    }

    type User3 {

        required property nick: str;

        required property name: str {

            # error: InvalidReferenceError: __source__ cannot be used in this expression
            default := (__source__.nick);

            # rewrite insert using (select __subject__.nick);

            # Error: invalid property reference on a primitive type expression
            # -> __subject__ is User3.name
            # constraint expression on (__subject__.name like 'a%');
        };

        trigger name_insert after insert for each do (
            assert_single((select __new__.nick))
        );
        trigger name_insert_all after insert for all do (
            assert_single((select __new__.nick))
        );


        access policy hide_unnamed
            allow select
            using (__subject__.nick != "");
        access policy write
            allow insert, update, delete;

        index on (__subject__.name);

        constraint exclusive on (__subject__.name);

        # Error: __subject__ cannot be used in this expression
        single property upper_name := str_upper(__source__.name);
    }
    
}

@msullivan
Copy link
Member

That's a great table! Could you finish it up with indexes, constraints, and computeds?

@msullivan
Copy link
Member

I think that User.property in FOR EACH is properly detached
When I try

    type User2 {
        required property nick: str;
        trigger name_insert after insert for each do (
            { single x := User2.nick }
        );
    }

I get

possibly more than one element returned by an expression for a computed property 'x' declared as 'single'

Using your version with a runtime assert_single I also got an error. Maybe you only tested that one when there wasn't actually other data?

@aljazerzen
Copy link
Contributor Author

aljazerzen commented Jun 12, 2023

I've filled in the table for the remaining constructs.

The User.property in "trigger for each" is indeed detached, my mistake. I've probably tested both triggers together and concluded that only one is throwing an error, instead of testing each separately.

@aljazerzen
Copy link
Contributor Author

Should I open a new issue about this?

@elprans
Copy link
Member

elprans commented Jun 12, 2023

@aljazerzen, computeds have the __source__ dunder, referring to the source object of the computed pointer.

@elprans
Copy link
Member

elprans commented Jun 12, 2023

(defaults too)

@aljazerzen
Copy link
Contributor Author

aljazerzen commented Jun 12, 2023

Edit: oh, you wrote __source__ not __subject__!

@elprans I'm getting these two errors:

type User3 {

    required property nick: str;

    required property name: str {

        # Error: __subject__ cannot be used in this expression
        default := (select __subject__.nick);
    };


    # Error: __subject__ cannot be used in this expression
    single property upper_name := str_upper(__subject__.name);
}

... it this only in SDL maybe? Or am I doing something wrong?

@aljazerzen
Copy link
Contributor Author

The default is still giving me errors, but only when I insert, which must be a bug:

edgedb> insert User3 { nick := "helllllo" };
error: InvalidReferenceError: __source__ cannot be used in this expression
  ┌─ <query>:1:1
  │
1 │ insert User3 { nick := "helllllo" };
  │ ^^^^^^^^^^ error

@aljazerzen aljazerzen merged commit 8488b4f into master Jun 12, 2023
21 checks passed
@aljazerzen aljazerzen deleted the fix-detached-defaults branch June 12, 2023 18:26
aljazerzen added a commit that referenced this pull request Jun 16, 2023
A followup for #5606 (comment)

This is techincally a breaking change, since any index definitions
that are using `Object.property` will now throw errors. I do think it is
quite minor and will probably not affect anyone, as this pattern is not documented
and treated as detached in all other SDL expressions apart from computeds.
aljazerzen added a commit that referenced this pull request Jun 16, 2023
Follow up for #5606 (comment).

Triggers that are not Delete, can now contain partial paths
that refer to `__new__`. Not a breaking change.
aljazerzen added a commit that referenced this pull request Jun 16, 2023
A followup for #5606 (comment)

This is techincally a breaking change, since any index definitions
that are using `Object.property` will now throw errors. I do think it is
quite minor and will probably not affect anyone, as this pattern is not documented
and treated as detached in all other SDL expressions apart from computeds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ISE when inserting defaults and refering to parent object type
3 participants