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

feat: support schema version 1.2 for modular models #1443

Merged
merged 10 commits into from
Mar 19, 2024

Conversation

ewanharris
Copy link
Member

Description

This PR adds support for using the schema version 1.2 behind the enable-modular-models experimental flag, and brings in the versions of openfga/api and openfga/language that support modular models.

As I was writing the additions I was thinking that the process of adding a new version isn't really scalable with the checks but I'm not familiar enough with the codebase to suggest any potential improvements here. Would love any feedback/prior consideration folks have had on this and would be happy to integrate into this PR.

Opening as a draft for now mostly just so the PR is here.

References

Closes #1402

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@ewanharris ewanharris linked an issue Mar 12, 2024 that may be closed by this pull request
1 task
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.02%. Comparing base (d7cd331) to head (22fbf8b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
+ Coverage   85.95%   86.02%   +0.07%     
==========================================
  Files          85       85              
  Lines        8049     8057       +8     
==========================================
+ Hits         6918     6930      +12     
+ Misses        796      793       -3     
+ Partials      335      334       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@miparnisari miparnisari left a comment

Choose a reason for hiding this comment

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

We had a similar need in the past: #613

We introduced two flags, one that allowed you to write models, and one that allowed you to call Check/ListObjects.

I think we should do something similar here.

Copy link
Contributor

@poovamraj poovamraj left a comment

Choose a reason for hiding this comment

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

Should we wrap the below line into it's own method so that it can be reused. Any future versions to be supported can be added here. Any version to be dropped from can be removed from here.

t.GetSchemaVersion() == SchemaVersion1_1 || t.GetSchemaVersion() == SchemaVersion1_2)

internal/graph/check.go Outdated Show resolved Hide resolved
@ewanharris
Copy link
Member Author

Ah thanks for raising that @miparnisari, I'll refactor the existing one to be enable-modular-models-write and add in a new enable-modular-models-evaluate.

@poovamraj Yeah I was thinking simplifying it would make sense, @jon-whit's suggestion makes sense to me and allows us handle if the feature is enabled a little easier.

I'll work on the above and once those are done I think this should be ready for review

@rhamzeh
Copy link
Member

rhamzeh commented Mar 15, 2024

We had a similar need in the past: #613

We introduced two flags, one that allowed you to write models, and one that allowed you to call Check/ListObjects.

I think we should do something similar here.

I don't think we need to. Unlike Schema 1.0, we're not planning on dropping support for 1.1.

All this new flag is doing is allowing support for 1.2. We expect to have a release out and remove that flag in a couple of weeks.

So as far as the server is concerned, only WriteAuthorizationModel will be concerned with rejecting 1.2 if the experimental flag is enabled. All the other pieces will treat 1.2 as supported.

That means if you can write the model, you can take all actions using it.

@ewanharris ewanharris marked this pull request as ready for review March 15, 2024 23:28
@ewanharris ewanharris requested a review from a team as a code owner March 15, 2024 23:28
miparnisari
miparnisari previously approved these changes Mar 15, 2024
rhamzeh
rhamzeh previously approved these changes Mar 15, 2024
@ewanharris ewanharris dismissed stale reviews from rhamzeh and miparnisari via d7787bf March 18, 2024 14:56
pkg/server/server_test.go Outdated Show resolved Hide resolved
pkg/server/server_test.go Show resolved Hide resolved
pkg/server/commands/write_authzmodel_test.go Show resolved Hide resolved
pkg/server/commands/write_authzmodel_test.go Show resolved Hide resolved
@rhamzeh rhamzeh merged commit 6a74f0f into main Mar 19, 2024
13 of 15 checks passed
@rhamzeh rhamzeh deleted the feat/1402-schema-12-support branch March 19, 2024 16:33
@willvedd willvedd mentioned this pull request Apr 10, 2024
4 tasks
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.

Support Schema 1.2
6 participants