java: Parse Expressions and Operators returned from the polar core. #1011

MFAshby · 2021-07-12T20:02:08Z

Remove test for throwing assertion when Expression is returned.

Ports tests for partial evaluation from the python code to java.

#1010

PR checklist:

Added changelog entry.
-> I'm not sure of the format yet, so I haven't done this.

github-actions · 2021-07-12T20:02:24Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

MFAshby · 2021-07-12T20:03:52Z

I have read the CLA Document and I hereby sign the CLA

Remove test for throwing assertion when Expression is returned. Add test for Expression successfully returned and parsed from polar core. #1010

…to java library. #1010

Ported the remaining relevant test testPartialConstraint from python to java. #1010

MFAshby · 2021-07-12T21:51:33Z

I've ported over the relevant tests from the python module, I welcome suggestions for more testing though.

Previous commits to add support for Operators mean that this is now supported in the java library. Fixes: #902

gj

This is awesome! To my currently very tired eyes, this looks like a great start. I'm out tomorrow, but I'll make sure that this either (A) gets looked at more closely by someone else tomorrow or (B) is first thing on my plate Thursday morning.

dhatch · 2021-07-14T17:57:03Z

@MFAshby added an acceptExpression flag on the Host class. We use this to protect users that are not expecting to receive a partial result from receiving expressions in the query output. All the python adapters set this flag to true when running queries, but by default it is False. The equivalent functionality in Python is here: https://github.com/osohq/oso/blob/main/languages/python/oso/polar/host.py#L237

samscott89

A few comments/suggestions.

languages/java/oso/src/main/java/com/osohq/oso/Exceptions.java

samscott89 · 2021-07-14T18:04:23Z

languages/java/oso/src/main/java/com/osohq/oso/Polar.java

+  public Query query(Predicate query, Map<String, Object> bindings, boolean acceptExpression)
+      throws Exceptions.OsoException {
+    Host new_host = host.clone();
+    new_host.setAcceptExpression(acceptExpression);


Why don't we make setAcceptExpression a method on the Query object that calls this? And then we don't need the multiple APIs for it?

Unfortunately Query gets a result when it is constructed from the core, so this parameter needs to be set on Host before query construction.

This also (roughly) matches the API in other languages, but I agree it's not ideal.

samscott89 · 2021-07-14T18:05:32Z

languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java

+
+    Predicate rule = new Predicate("f", List.of(x));
+    List<HashMap<String, Object>> results =
+        p.query(rule, Map.of("x", new TypeConstraint(x, "User")), true).results();


How is this working if ToPolar isn't implemented for it?

It's possible that it's not working, this might be the reason for 2 results instead of 1

This is working now, I added the implementation to the PR. I noticed this test was different though as ported. The python test expects 1 result.

Are there any other tests you changed while porting?

Not to my knowledge. I erroneously thought this should return 2 results for some reason.

Cool, I think it's just because the python test looks for two arguments in the And expression. I got a bit confused myself reading it.

I checked the other tests & they look good to me!

samscott89 · 2021-07-14T18:07:08Z

languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java

+    Predicate rule = new Predicate("f", List.of(x));
+    List<HashMap<String, Object>> results =
+        p.query(rule, Map.of("x", new TypeConstraint(x, "User")), true).results();
+    assertEquals(2, results.size());


This should only have one result from the rule matching User, not the rule matching Post.

MFAshby

Thanks @dhatch ! I didn't know the motivation behind that argument, but it makes sense now.

languages/java/oso/src/main/java/com/osohq/oso/Exceptions.java

MFAshby · 2021-07-14T20:05:47Z

languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java

+
+    Predicate rule = new Predicate("f", List.of(x));
+    List<HashMap<String, Object>> results =
+        p.query(rule, Map.of("x", new TypeConstraint(x, "User")), true).results();


It's possible that it's not working, this might be the reason for 2 results instead of 1

dhatch · 2021-07-14T20:46:49Z

It's possible that it's not working, this might be the reason for 2 results instead of 1

The same test in Python is also returning two results, but I would expect it to return one. I'm going to do a little investigation on that. I'll update you tomorrow @MFAshby!

languages/java/oso/src/main/java/com/osohq/oso/Exceptions.java

And add isSubclass event.

languages/java/oso/src/test/java/com/osohq/oso/OsoTest.java

MFAshby · 2021-07-15T21:48:11Z

languages/java/oso/src/main/java/com/osohq/oso/Host.java

@@ -191,6 +226,29 @@ public JSONObject toPolarTerm(Object value) throws Exceptions.OsoException {
          "Call", new JSONObject(Map.of("name", pred.name, "args", javaListToPolar(pred.args))));
    } else if (value != null && value instanceof Variable) {
      jVal.put("Variable", value);
+    } else if (value != null && value instanceof Expression) {
+      Expression expression = (Expression) value;


Thanks for this. I completely missed transforming in this direction.

samscott89

LGTM

MFAshby · 2021-07-16T08:16:57Z

Does the changelog need updating here or in another PR?

MFAshby added 3 commits July 12, 2021 21:17

java: Parse Expressions and Operators returned from the polar core.

2f5701f

Remove test for throwing assertion when Expression is returned. Add test for Expression successfully returned and parsed from polar core. #1010

Ported a couple of the partial evaluation tests over from the python …

130fcfb

…to java library. #1010

java: Add bindings when creating a query.

167d3c4

Ported the remaining relevant test testPartialConstraint from python to java. #1010

MFAshby marked this pull request as ready for review July 12, 2021 21:51

Add simple test for comparison operators in Oso.

377fc44

Previous commits to add support for Operators mean that this is now supported in the java library. Fixes: #902

gj reviewed Jul 14, 2021

View reviewed changes

dhatch added 2 commits July 14, 2021 13:52

Add accept expression flag.

4ada527

format.

aa9d1bf

samscott89 reviewed Jul 14, 2021

View reviewed changes

MFAshby commented Jul 14, 2021

View reviewed changes

gj reviewed Jul 15, 2021

View reviewed changes

languages/java/oso/src/main/java/com/osohq/oso/Exceptions.java Outdated Show resolved Hide resolved

Add toPolarTerm for Expression and Pattern.

dc7488a

And add isSubclass event.

dhatch reviewed Jul 15, 2021

View reviewed changes

languages/java/oso/src/test/java/com/osohq/oso/OsoTest.java Outdated Show resolved Hide resolved

dhatch and others added 3 commits July 15, 2021 16:13

Update languages/java/oso/src/test/java/com/osohq/oso/OsoTest.java

2d09ca3

Correct spelling in error messages.

f8f1677

Add unexpected expression test.

f9b55ca

MFAshby commented Jul 15, 2021

View reviewed changes

samscott89 approved these changes Jul 15, 2021

View reviewed changes

dhatch and others added 2 commits July 15, 2021 18:48

Format.

08f1201

Merge branch 'main' into main

45f377b

gj and others added 3 commits July 16, 2021 12:07

Merge branch 'main' into main

e7b0561

Fix singleton errors

3c4d93b

Add changelog entry crediting @MFAshby

530fade

gj merged commit 41aeaed into osohq:main Jul 16, 2021

github-actions bot locked and limited conversation to collaborators Jul 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

java: Parse Expressions and Operators returned from the polar core. #1011

java: Parse Expressions and Operators returned from the polar core. #1011

MFAshby commented Jul 12, 2021 •

edited

github-actions bot commented Jul 12, 2021 •

edited

MFAshby commented Jul 12, 2021

MFAshby commented Jul 12, 2021

gj left a comment

dhatch commented Jul 14, 2021

samscott89 left a comment

samscott89 Jul 14, 2021

dhatch Jul 15, 2021

samscott89 Jul 14, 2021

MFAshby Jul 14, 2021

dhatch Jul 15, 2021

MFAshby Jul 15, 2021

dhatch Jul 15, 2021

samscott89 Jul 14, 2021

MFAshby left a comment

MFAshby Jul 14, 2021

dhatch commented Jul 14, 2021

MFAshby Jul 15, 2021

samscott89 left a comment

MFAshby commented Jul 16, 2021

java: Parse Expressions and Operators returned from the polar core. #1011

java: Parse Expressions and Operators returned from the polar core. #1011

Conversation

MFAshby commented Jul 12, 2021 • edited

github-actions bot commented Jul 12, 2021 • edited

MFAshby commented Jul 12, 2021

MFAshby commented Jul 12, 2021

gj left a comment

Choose a reason for hiding this comment

dhatch commented Jul 14, 2021

samscott89 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

MFAshby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhatch commented Jul 14, 2021

Choose a reason for hiding this comment

samscott89 left a comment

Choose a reason for hiding this comment

MFAshby commented Jul 16, 2021

MFAshby commented Jul 12, 2021 •

edited

github-actions bot commented Jul 12, 2021 •

edited