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

Include reasoning logic when restricting db #767

Merged
merged 9 commits into from
Jun 6, 2024

Conversation

JaceRockman
Copy link
Contributor

No description provided.

@JaceRockman JaceRockman requested a review from bplatz May 9, 2024 15:00
@JaceRockman
Copy link
Contributor Author

JaceRockman commented May 9, 2024

This is meant to solve Story 1 from fluree/core#102

Accepts a rules graph with :reasoner-rules in the opts and a rules db
with :reasoner-rules-db. Both of these keys expect a list, but since
our reasoner only supports one rule source currently, the list should
only have one element. If both a graph and a db are provided, it will
use the db.
@JaceRockman JaceRockman marked this pull request as draft May 13, 2024 17:05
@JaceRockman JaceRockman requested a review from a team May 13, 2024 19:30
@JaceRockman
Copy link
Contributor Author

If anyone has ideas on how to get a db of rules from just an alias when using query instead of query-connection, let me know, otherwise, I think this is good to merge.

@JaceRockman JaceRockman marked this pull request as ready for review May 13, 2024 19:32
Copy link
Contributor

@cap10morgan cap10morgan 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 overall! I noted a couple of very minor formatting things to address just in the interest of minimizing whitespace churn in the future. Thanks!

src/clj/fluree/db/api/query.cljc Outdated Show resolved Hide resolved
src/clj/fluree/db/api/query.cljc Outdated Show resolved Hide resolved
db)
time-travel-db (-> (if t
(<? (time-travel/as-of policy-db t))
policy-db))
Copy link
Contributor

Choose a reason for hiding this comment

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

Love these more descriptive var names!

Copy link
Contributor

Choose a reason for hiding this comment

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

With the new policy PR, this has changed a little bit so the above wouldn't be compatible. Now you compose dbs, instead of a bunch of if conditions in the API. In this case if you wanted a db to be policy-wrapped, you'd call that API first explicitly. This no longer looks for this option in a regular query.

Also if a query is wrapped in a verifiable credential, there is a new explity query API for that... so this check won't be done always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that PR has failing checks, should I wait for it to be merged and then adjust this PR to work with it?

@JaceRockman JaceRockman merged commit 56551f1 into main Jun 6, 2024
7 checks passed
@JaceRockman JaceRockman deleted the feature/query-with-reasoners branch June 6, 2024 16:19
@JaceRockman JaceRockman restored the feature/query-with-reasoners branch June 6, 2024 16:22
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

3 participants