feat: Wrap types from `rio` to have views and normal owning types #23

supermaxiste · 2024-07-01T15:16:41Z

Proposed Changes

Draft: adding new types from rio using String instead of &str

Types of Changes

What types of changes does your code introduce? Put an x in the boxes that
apply

A bug fix (non-breaking change which fixes an issue).
A new feature (non-breaking change which adds functionality).
A breaking change (fix or feature that would cause existing
functionality to not work as expected).
A non-productive update (documentation, tooling, etc. if none of the
other choices apply).

Checklist

I have read the
CONTRIBUTING
guidelines.
I have added tests that prove my fix is effective or that my feature
works.
I have added the necessary documentation (if appropriate).

Further Comments

If you could check both rdf_types.rsand model.rs that would be great. Before moving forward I need to see if you're fine with this solution. Important notes:

The Copy trait doesn't work anymore, I think because we're not dealing with&str anymore?
We cannot include RDF-Star in our types because it causes a loop in our definition that Rust doesn't like. For example Triple including Subject and Subject including Triple. This cycle breaks the query system: https://rustc-dev-guide.rust-lang.org/query.html. The only way to avoid it is to use &str
I had to bring over the Display functions too, otherwise the rest doesn't work

src/model.rs

src/rdf_types.rs

src/model.rs

supermaxiste · 2024-07-08T10:00:46Z

Update

Added rules in rules.rs
Added many unit tests to ensure rules do what they're supposed to
Fixed is_set function and added unit test to make it works as expected

supermaxiste · 2024-07-08T10:01:28Z

Addresses #5

gabyx

Good job with the tests and rules in general. Before you start with the review read: It might more sense to sit together and pair-program this, which is where we both learn =).

There are some comments which need some discussion =).

Logic:

The eval_ function do their job, but have strange signature and there is some input we should go through together.
The serialization is of the rdf_types might still be a bit raw and can be niceified in another PR.

Style:

I am not sure what rust-fmt does, but I guess its not as hard on the newlines as it shoud be. Some parts in rules.rs are glued together which is not nice for the reader =). I try to show you what I mean while pair-programming this.

There is more... -> Metting

gabyx · 2024-07-08T12:05:31Z

src/crypto.rs

+            Entity::BlankNode(b) => Entity::BlankNode(self.pseudo_blank_node(&b)),
+        }
+    }
+    // private methods? Blanket implementations


hint: Is this a comment? Otherwise remove.

gabyx · 2024-07-08T12:06:01Z

src/crypto.rs

+    //return l.clone();
+
+    fn pseudo_blank_node(&self, u: &BlankNode) -> BlankNode;
+    // return u.clone()


hint: remove

Suggested change

// return u.clone()

gabyx · 2024-07-08T12:06:24Z

src/crypto.rs

+    }
+    // private methods? Blanket implementations
+    fn pseudo_named_node(&self, t: &NamedNode) -> NamedNode;
+    //return t.clone();


Suggested change

//return t.clone();

gabyx · 2024-07-08T12:06:34Z

src/crypto.rs

+    //return t.clone();
+
+    fn pseudo_literal(&self, l: &Literal) -> Literal;
+    //return l.clone();


Suggested change

//return l.clone();

gabyx · 2024-07-08T12:15:18Z

src/crypto.rs

+impl Pseudonymize for DefaultHasher {
+    fn pseudo_named_node(&self, t: &NamedNode) -> NamedNode {
+        // We check for the last backslash in the IRI and add 1 to include the backslash
+        let prefix = &t.iri[0..t.iri.rfind('/').unwrap() + 1];


question: What happens when there is no /. Good documented.
Should the function panic or gracefully ignore the not-found prefix?

suggetion: unwrap -> panic. maybe wrap this with expect("Expected to find a '<prefix>/...'")

otherwise: What we can and should look into is how to correctly split this: LINK
I assume rio does not provide and facility to deal with IRIs...?

gabyx · 2024-07-08T12:52:14Z

src/rules.rs

+            mask = eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);
+            return mask;


Suggested change

mask = eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);

return mask;

return eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);

gabyx · 2024-07-08T12:52:41Z

src/rules.rs

+                mask |= TripleMask::OBJECT;
+                return mask;


Suggested change

mask |= TripleMask::OBJECT;

return mask;

return mask | TripleMask::OBJECT;

gabyx · 2024-07-08T12:53:17Z

src/rules.rs

+            } else {
+                return mask;
+            }


Suggested change

} else {

return mask;

}

}

return mask;

gabyx · 2024-07-08T12:55:59Z

src/rules.rs

+        let mut type_map = HashMap::new();
+        type_map.insert("http://example.org/Alice".to_string(), "Person".to_string());
+        let mut mask = TripleMask::new();
+        mask = eval_type_rule_object(&object, mask, &type_map, &rules);


suggestion: I would rather go for a name match_rule_object which makes more sense, IMO. eval -> suggests you are already executing this rule.

gabyx · 2024-07-08T12:58:18Z

src/rules.rs

+            } else {
+                return mask;
+            };
+            if rules.replace_values_of_subject_predicate.contains_key(subject_type) {


hint: we need to pair program this 🤣 > Have some important input here. About nesting and glueing and
how the signature of this function should be =)

- Remove nesting and group types and impls together Signed-off-by: Gabriel Nützi <gnuetzi@gmail.com>

- added rstest for table driven tests.

supermaxiste requested review from gabyx and cmdoret July 2, 2024 08:05

cmdoret reviewed Jul 2, 2024

View reviewed changes

src/model.rs Outdated Show resolved Hide resolved

gabyx requested changes Jul 2, 2024

View reviewed changes

gabyx changed the title ~~Feat/hash~~ feat: Wrap types from rio to have views and normal owning types Jul 6, 2024

supermaxiste requested review from cmdoret and gabyx July 8, 2024 10:01

supermaxiste self-assigned this Jul 8, 2024

supermaxiste marked this pull request as ready for review July 8, 2024 10:03

gabyx requested changes Jul 8, 2024

View reviewed changes

supermaxiste and others added 10 commits July 10, 2024 10:18

draft: hashing test

e865f6f

fix: from function to convert pointer to boxed string

cbbfa6c

fix: clean crypto

12944e7

fix: managed hashing?

4bcf2f1

refactor: types for rdf protect to rio types

31026a8

fix: add new pseudo structure

3ddbb82

fix: match types and restructure pseudonymization call

9cd0b7d

fix: implemented subject rules

d66942a

fix: implemented rules with tests

144f895

fix: review changes

f731093

- Remove nesting and group types and impls together Signed-off-by: Gabriel Nützi <gnuetzi@gmail.com>

gabyx force-pushed the feat/hash branch from 1fcda32 to f731093 Compare July 10, 2024 08:19

gabyx added 2 commits July 10, 2024 10:37

fix: review changes

aa8e6df

- added rstest for table driven tests.

fix: remove obsolete brackets

61f162c

gabyx approved these changes Jul 10, 2024

View reviewed changes

supermaxiste merged commit 4c8b5c4 into main Jul 10, 2024
7 checks passed

supermaxiste deleted the feat/hash branch July 10, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Wrap types from `rio` to have views and normal owning types #23

feat: Wrap types from `rio` to have views and normal owning types #23

supermaxiste commented Jul 1, 2024

supermaxiste commented Jul 8, 2024

supermaxiste commented Jul 8, 2024

gabyx left a comment

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

gabyx Jul 8, 2024

		mask = eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);
		return mask;

	mask = eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);
	return mask;
	return eval_type_rule_named_node(false, n.clone(), mask, rules, type_map);

	mask \|= TripleMask::OBJECT;
	return mask;
	return mask \| TripleMask::OBJECT;

feat: Wrap types from rio to have views and normal owning types #23

feat: Wrap types from rio to have views and normal owning types #23

Conversation

supermaxiste commented Jul 1, 2024

Proposed Changes

Types of Changes

Checklist

Further Comments

supermaxiste commented Jul 8, 2024

Update

supermaxiste commented Jul 8, 2024

gabyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Wrap types from `rio` to have views and normal owning types #23

feat: Wrap types from `rio` to have views and normal owning types #23