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

make sure that we actually need to run the datalog engine #197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Nov 12, 2023

the authorizer needs the datalog engine to have executed before performing certain tasks, but we might have already done it precedently, without having added facts or rules.
This adds a boolean tracking if we need to run: if we added facts or rules, it is set to true, if we have just run the datalog engine, it is set to false

the authorizer needs the datalog engine to have executed before
performing certain tasks, but we might have already done it precedently,
without having added facts or rules.
This adds a boolean tracking if we need to run: if we added facts or
rules, it is set to true, if we have just run the datalog engine, it is
set to false
Copy link
Collaborator

@divarvel divarvel left a comment

Choose a reason for hiding this comment

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

Looks good, i'd be more comfortable with tests exercising it though

@@ -218,33 +222,38 @@ impl Authorizer {
/// Add the rules, facts, checks, and policies of another `Authorizer`.
/// If a token has already been added to `other`, it is not merged into `self`.
pub fn merge(&mut self, mut other: Authorizer) {
self.dirty = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one is redundant, but better safe than sorry, I guess

Copy link

codspeed-hq bot commented May 26, 2024

CodSpeed Performance Report

Merging #197 will not alter performance

Comparing geal/prevent-reruns (99b3824) with main (71f4350)

Summary

✅ 12 untouched benchmarks

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 80.76923% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 67.48%. Comparing base (71f4350) to head (99b3824).

Files Patch % Lines
biscuit-auth/src/token/authorizer.rs 79.16% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #197   +/-   ##
=======================================
  Coverage   67.47%   67.48%           
=======================================
  Files          25       25           
  Lines        5347     5370   +23     
=======================================
+ Hits         3608     3624   +16     
- Misses       1739     1746    +7     

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

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.

None yet

2 participants