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

Cast expression suggestion when changing type of a link property references it as a standard property #1188

Open
raddevon opened this issue Dec 12, 2023 · 3 comments · May be fixed by #1197
Labels
bug Something isn't working

Comments

@raddevon
Copy link
Contributor

When altering the type of a link property as shown in the schema at the bottom:

did you alter the type of property 'date' of link 'habit_completed'? [y,n,l,c,b,s,q,?]
> y
Please specify a conversion expression to alter the type of property 'date':
cast_expr> <cal::local_date>.date

Suggestion is to cast a property .date which does not exist on the type. The suggestion should be <cal::local_date>@date instead.

  • EdgeDB Version: 5.0-dev.8091+e24f7fd
  • EdgeDB CLI Version: 4.0.2+500be79
  • OS Version: macOS 14.0

Steps to Reproduce

  1. Create a schema with a link property
  2. Migrate
  3. Change the type of the link property
  4. Migrate again

Schema

Start:

module default {
    type Habit {
      required name: str;
    }
    type User {
      name: str;
      multi habit_completed: Habit {
        date: datetime;
        # Ensures a User can complete a given Habit
        # only once per day.
        constraint exclusive on ((@target, @date));
      }
    }
}

End:

module default {
    type Habit {
      required name: str;
    }
    type User {
      name: str;
      multi habit_completed: Habit {
        date: cal::local_date;
        # Ensures a User can complete a given Habit
        # only once per day.
        constraint exclusive on ((@target, @date));
      }
    }
}
@raddevon raddevon added the bug Something isn't working label Dec 12, 2023
@Dhghomon
Copy link
Contributor

Dhghomon commented Dec 13, 2023

Interesting issue, noticed a few things:

  • The original schema doesn't work but does if you constrain on just @date (and reproduces the error)
  • I think this is an edgedb and not edgedb-cli issue because the CLI just uses create migration to (schema) and then describe current migration as json which gives it this prompt from the server which it simply follows:
  "parent": "m1t2it77gg4vsiznipiim453v3b7z6hhjy3v6j7e2xumflsmwjtc7q",
  "complete": false,
  "proposed": {
    "prompt": "did you alter the type of property 'date' of link 'habit_completed'?",
    "data_safe": false,
    "prompt_id": "SetPropertyType PROPERTY default::__|date@default|__||habit_completed&default||User",
    "confidence": 1.0,
    "statements": [
      {
        "text": "ALTER TYPE default::User {\n    ALTER LINK habit_completed {\n        ALTER PROPERTY date {\n            SET TYPE cal::local_date USING (\\(cast_expr));\n        };\n    };\n};"
      }
    ],
    "required_user_input": [
      {
        "prompt": "Please specify a conversion expression to alter the type of property 'date'",
        "new_type": "cal::local_date",
        "old_type": "std::datetime",
        "placeholder": "cast_expr",
        "pointer_name": "date",
        "new_type_is_object": false,
        "old_type_is_object": false
      }
    ]
  },
  "confirmed": []
}

(Correction: looks like this is indeed a CLI issue: per @msullivan the server just reports the old and new type) https://github.com/edgedb/edgedb-cli/blob/master/src/migrations/create.rs#L332

The suggested cast is produced by the CLI, not by the compiler/server. The server just reports the old_type and the new_type back to the CLI

  • Casting into a cal::local_date won't work in any case whereas something like this will: cal::to_local_date(@date, 'UTC') So maybe the prompt should be cal::to_local_date(@date, <timezone>) or something like that? Since the local_date will vary depending on which timezone is being used as the local time.

@raddevon
Copy link
Contributor Author

You're correct that the original cast suggestion won't work, even if it references the property correctly, but I wasn't sure how sophisticated and bespoke those suggestions could be. Yeah, this suggestion you made (cal::to_local_date(@date, <timezone>)) is basically what I ended up using to do the cast.

So, maybe this is actually two issues:

  1. reference link properties properly when giving a cast expression suggestion and
  2. give a working suggestion when changing a datetime property to a cal::local_date

@Dhghomon
Copy link
Contributor

You're correct that the original cast suggestion won't work, even if it references the property correctly, but I wasn't sure how sophisticated and bespoke those suggestions could be. Yeah, this suggestion you made (cal::to_local_date(@date, <timezone>)) is basically what I ended up using to do the cast.

So, maybe this is actually two issues:

  1. reference link properties properly when giving a cast expression suggestion and
  2. give a working suggestion when changing a datetime property to a cal::local_date

I was looking into this today and looks like it's pretty easy to just query the database with select schema::Cast and select schema::Function and then put together a HashMap to tell which casts are doable, and if not castable then which functions could work. So I think I can make this more intuitive in addition to the link property fix.

@Dhghomon Dhghomon linked a pull request Jan 24, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants