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(query): privilege type forward compat #14501

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Jan 29, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

  1. add ci test foreward compat test(now only test rbac)
  2. UserPrivilegeType and ShareGrantObjectPrivilege deserialize use from_bits_truncate

Befero we use from_bits, in this pr, use from_bits_truncate

from_bits will cause breaking change. When add new bit, old query can not deserialize the new bit. And then return err.

https://docs.rs/enumflags2/0.7.7/enumflags2/struct.BitFlags.html#method.from_bits

https://docs.rs/enumflags2/0.7.7/enumflags2/struct.BitFlags.html#method.from_bits_truncate

I think we should ignore the new bit. So now call from_bits_truncate to deserialize bitflags.

  • Fixes #[Link the issue here]

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Jan 29, 2024
@TCeason TCeason force-pushed the fix_built_in_role branch 2 times, most recently from a9052d1 to 2f736d1 Compare January 29, 2024 03:47
@TCeason TCeason requested a review from everpcpc January 29, 2024 03:49
@TCeason
Copy link
Collaborator Author

TCeason commented Jan 29, 2024

@everpcpc Please help to review the .github yaml and test-foreward-compat part in this pr.

@TCeason TCeason force-pushed the fix_built_in_role branch 2 times, most recently from 5348d13 to 1d8861a Compare January 29, 2024 05:01
@TCeason
Copy link
Collaborator Author

TCeason commented Jan 29, 2024

cc @lichuang @drmingdrmer Are there any other concerns about considering using bits_from? Through these tests added so far, I think it's safe to guarantee that the current pr will not cause a break. Please review it.

cc @BohuTANG Maybe can add a ci-cloud tag, and try to fix your warehouse?

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @lichuang, @TCeason, and @Xuanwo)


tests/fuse-forward-compat/util.sh line 1 at r1 (raw file):

#!/bin/bash

Do not copy, reuse util.sh. It's a burden maintaining two copies of the same function set.

@drmingdrmer
Copy link
Member

@TCeason
IMHO, You can just add the forward compat test in the fuse-compat dir.

@TCeason
Copy link
Collaborator Author

TCeason commented Jan 29, 2024

@TCeason IMHO, You can just add the forward compat test in the fuse-compat dir.

I got you. I'm modifing it. But now this can prove that this pr not generate new break change?

@TCeason
Copy link
Collaborator Author

TCeason commented Jan 29, 2024

@TCeason IMHO, You can just add the forward compat test in the fuse-compat dir.

Done

1. add ci test forward compat test(now only test rbac)
2. UserPrivilegeType and ShareGrantObjectPrivilege deserialize use from_bits_truncate
@drmingdrmer
Copy link
Member

@TCeason
Why do not you just re-use run_test in the util.sh but create another run_forward_test instead?
These two function does the same thing: write to version A and ready with version B.
Just update variable names run_test uses and reuse it for forward compatibility test.

@TCeason TCeason force-pushed the fix_built_in_role branch 3 times, most recently from bcd9bbd to 7c1a7f3 Compare January 29, 2024 08:30
@TCeason TCeason added the ci-cloud Build docker image for cloud test label Jan 29, 2024
Copy link
Contributor

Docker Image for PR

  • tag: pr-14501-e43569a

note: this image tag is only available for internal use,
please check the internal doc for more details.

@BohuTANG BohuTANG merged commit 0c05f06 into databendlabs:main Jan 29, 2024
76 of 77 checks passed
@BohuTANG
Copy link
Member

Test on Cloud Prod, user auth has no issue.

TCeason added a commit to TCeason/datafuse that referenced this pull request Aug 26, 2024
* fix(query): privilege type forward compat

1. add ci test forward compat test(now only test rbac)
2. UserPrivilegeType and ShareGrantObjectPrivilege deserialize use from_bits_truncate

* run_test contain forward and backward
TCeason added a commit to TCeason/datafuse that referenced this pull request Aug 26, 2024
* fix(query): privilege type forward compat

1. add ci test forward compat test(now only test rbac)
2. UserPrivilegeType and ShareGrantObjectPrivilege deserialize use from_bits_truncate

* run_test contain forward and backward
TCeason added a commit to TCeason/datafuse that referenced this pull request Aug 26, 2024
* fix(query): privilege type forward compat

1. add ci test forward compat test(now only test rbac)
2. UserPrivilegeType and ShareGrantObjectPrivilege deserialize use from_bits_truncate

* run_test contain forward and backward
yufan022 added a commit to yufan022/databend that referenced this pull request Sep 9, 2024
yufan022 added a commit to yufan022/databend that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-bugfix this PR patches a bug in codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants