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

EdgeDB Project Context Schema #2931

Merged
merged 16 commits into from
Nov 8, 2023
Merged

EdgeDB Project Context Schema #2931

merged 16 commits into from
Nov 8, 2023

Conversation

CarsonF
Copy link
Member

@CarsonF CarsonF commented Nov 7, 2023

Why

This builds upon #2922. It established the pattern for how a project's sensitivity and current user's membership is shared for all types under a project, engagement, products, partnerships, etc.

However, we still have the use case of Languages, Partners, etc. These also use their related project's sensitivity and membership to determine authorization. It's just that they need aggregate multiple projects instead of a single one.

I had isMember and effectiveSensitivity defined on Language, and this is useful when we are looking at a language directly.

select Language {*}
filter .effectiveSensitivity <= Sens.Medium

This would indeed work for language queries and access policies. So I thought I had the problem solved, though admittedly a bit clumsy for these handful of types that were "project aware but outside projects". Since they would need to declare their own sensitivity and isMember fields themselves.

I started working on File schema, and wanted something that also works for Posts/Comments which are embedded into other resources. Now if I wanted to say "this directory can only be read if it's parent's sensitivity is medium or lower" it was impossible.
Every type that had a computed sensitivity property would need to be enumerated.
Something like

select Object {
  sensitivity := (
    [is Project].sensitivity ??
    [is Language].sensitivity ??
    [is Partner].sensitivity ??
    [is Organization].sensitivity
  )
}

This is not good.
We needed a way to refer to a single sensitivity field.

To do this we needed a unified stored field that all project based queries could point back to.

Project Context (What)

This adds a Project::Context type which holds a list of projects.
All project (context) aware objects will now have a reference to one of these:

  • For Project English 1, its project context list would just be itself:
    { English 1 }
  • For Language English, its project context list would be all projects engaged with it:
    { English 1, English 2, English 3 }
  • For Partner Seed Co, its project context list would be all projects its partnered with:
    { English 1, Spanish 1 }

Now Language, Project, Engagement all extend from Project::ContextAware. This abstract type is now able to provide a single computed field for sensitivity that all 3 of these concrete types can use.
Looking back at our query above, it can now be

select Object {
  [is Project::ContextAware].sensitivity
}

Draw backs

I've prioritized reading back these values, as that is the main need. For app queries, access policies, and adhoc reads from DB UI.
In order to make this happen though, this project context list is (unfortunately) denormalized from the actual relations that declare these connections.
So it's up to the write process to declare how these different relations should affect project contexts.
For languages (and partners in future), this is done with triggers (which you can see I've added here). So queries do not need to worry about it.
You can also see that Ethnologue::Language now actually supports these as well. It shares the project context from the related default::Language so it gets those changes automatically.

Projects is the main hiccup. Currently edgedb cannot "link to self" with a single insert operation. Since a project's context is a list of itself, this is a problem. So for now, when creating projects, the project needs to be created first, and the separately, secondly updated to add itself to its context. But this will be done once in app code, and will just have to be something to remember if doing adhoc project creation.
This is by far the biggest rough edge, and I'll keep an eye out for a better (schema declared) workaround.

ownSensitivity

With sensitivity being the single computed field on all of these types, we needed a different stored field to be the source.
I called this ownSensitivity, aka a Language's own sensitivity. Where as the "effective sensitivity" for authorization is now pulled from sensitivity.
This, I think, is more clear too. ownSensitivity is always writable, and sensitivity is always computed.
Project::ContextAware declares this field as optional, but Project & Language overload to make it required.

Project::Child

I renamed Project::Resource -> Project::Child to hopefully be a bit more clear.
Also Engagement::Resource -> Engagement::Child.
Still not 100% sold, but could be better...maybe.

EdgeDB 4.0

Also bumped to v4 lol

┆Issue is synchronized with this Monday item by Unito

Copy link
Contributor

@bryanjnelson bryanjnelson left a comment

Choose a reason for hiding this comment

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

Soooo much progress.

@@ -57,7 +57,7 @@ module default {

trigger confirmProjectSens after update for each do (
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal pet peeve...sensitivity is spelled out 90% of the places...why not make it consistent and spell it out everywhere? It doesn't add much space to do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that mean you don't like this either? 🙈

scalar type Sens extending Sensitivity;

I can spell out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't, but there's more of a reason to do it there. Feel free to ignore.

@CarsonF CarsonF marked this pull request as ready for review November 8, 2023 03:45
@CarsonF CarsonF merged commit ac6332b into develop Nov 8, 2023
14 checks passed
@CarsonF CarsonF deleted the edgedb/project-context branch November 8, 2023 20:23
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.

2 participants