Skip to content

[db] fix the query for non-fga migrated users #19194

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

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

svenefftinge
Copy link
Contributor

@svenefftinge svenefftinge commented Dec 5, 2023

Description

Fixes the query for listing users that should be migrated by the FGA job.
At the moment the query is wrong and returns empty lists. As a consequence many users that haven't been active recently are not considered by FGA and can therefore not be found using our admin dashboard.

Summary generated by Copilot

🤖[deprecated] Generated by Copilot at d43464b

Improve FGA migration query and add test case. The changes exclude built-in users from the query that selects users who need to migrate to a newer fine-grained access (FGA) version. The changes also add a unit test to verify the query logic and results.

Related Issue(s)

Fixes #EXP-1021

How to test

Documentation

Preview status

gitpod:summary

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft preemptible
    Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

@svenefftinge
Copy link
Contributor Author

/unhold

.createQueryBuilder("user")
.select(["id"])
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should be able to keep the select to reduce the amount of bytes shuffled around.

Copy link
Member

Choose a reason for hiding this comment

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

@svenefftinge I tried, and it's indeed this line that fails the method, .select(["user.id"]) works. 🙄 So this is different to other querybuilder methods, which does not auto-namespace it's argument. And also, MySQL happily executes and retunrs and empty result... 🙈

.createQueryBuilder("user")
.select(["id"])
.where({
fgaRelationshipsVersion: Not(Equal(fgaRelationshipsVersion)),
Copy link
Member

Choose a reason for hiding this comment

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

@svenefftinge Can you share some insight into what's not working here, e.g. how the resulting query looks like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I wrote a test and verified it failed and then used the pattern we use in other queries as well to make the test pass.

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Code looks good, tests are green ✔️

@roboquat roboquat merged commit ad2a077 into main Dec 5, 2023
@roboquat roboquat deleted the se/fix-fga-job branch December 5, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants