Added Array, Set and operator 'IN' #110

ystrict · 2023-06-19T13:36:11Z

Hi, I've added implementation for array, set and 'in' support. Please take a look on it, will be happy if you make some comments!

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

tgeens · 2023-06-21T08:52:34Z

I was going to ask to add a test in QuerySetToThunkExpressionConverterTest first, but that probably depends on xenit-eu/opa-java-client#12 ?

ystrict · 2023-06-23T13:10:43Z

QuerySetToThunkExpressionConverterTest

That's right. I can add default implementation (throwing UnsupportedException) to RegoVisitor (T visit(SetTerm setTerm);) not to fail the build of ThunX. And then add test to ThunX after merging opa-client

tgeens · 2023-06-26T09:06:45Z

@ystrict the opa-java-client PR has been merged. Can you update this PR accordingly ?

ystrict · 2023-06-26T10:37:47Z

@ystrict the opa-java-client PR has been merged. Can you update this PR accordingly ?

Done, but please take a look on this one firstly(made some small fixes) - xenit-eu/opa-java-client#26

ystrict · 2023-06-26T13:39:10Z

@tgeens can you please restart the build? looks like dependencies didn't get in time

ystrict · 2023-06-26T15:52:28Z

@tgeens Can you please take a look on this build issue? I've checked locally and it looks like something is wrong with the github publishing process of opa-client. I've checked your sonatype repo and it contains the correct version of opa-client with my last updates. Locally gradle gets the correct jars and so on, but it's not building and IDE says that the bytecode doesn't match the source code, it's strange

thunx-model/src/main/java/com/contentgrid/thunx/predicates/model/CollectionValue.java

tgeens · 2023-06-27T09:41:25Z

@tgeens Can you please take a look on this build issue? I've checked locally and it looks like something is wrong with the github publishing process of opa-client. I've checked your sonatype repo and it contains the correct version of opa-client with my last updates. Locally gradle gets the correct jars and so on, but it's not building and IDE says that the bytecode doesn't match the source code, it's strange

You should update the dependency to opa-java-client in thunx-bom/build.gradle from 0.4.0 to 0.4.1-SNAPSHOT

ystrict · 2023-06-27T09:45:59Z

@tgeens Can you please take a look on this build issue? I've checked locally and it looks like something is wrong with the github publishing process of opa-client. I've checked your sonatype repo and it contains the correct version of opa-client with my last updates. Locally gradle gets the correct jars and so on, but it's not building and IDE says that the bytecode doesn't match the source code, it's strange

You should update the dependency to opa-java-client in thunx-bom/build.gradle from 0.4.0 to 0.4.1-SNAPSHOT

done

tgeens · 2023-06-27T10:04:52Z

Ok, but that won't be sufficient. See my comment on CollectionValue.

ystrict · 2023-07-03T10:34:37Z

Ok, but that won't be sufficient. See my comment on CollectionValue.

Hi, can you please take a look on my changes?

ystrict · 2023-07-11T07:36:03Z

@tgeens Hi, I've changed the version but it's still falling. For me it looks like there are some building or publishing issues, because on local using publishToMavenLocal it works good. Can you please take a look and help me in your free time?

vierbergenlars · 2023-07-11T08:52:57Z

because on local using publishToMavenLocal it works good.

I found a compile error here: https://github.com/xenit-eu/thunx/pull/110/files#diff-e1519f085982147798370b05f00c49957d1c5f2be3db626be759193e055631a3R155

It's trying to pass Rego objects to something that should be a thunx Scalar.

It's strange that it compiles for you locally, do you still have some uncommitted changes, or perhaps an includeBuild() left in settings.gradle that is replacing some dependencies?

thunx-model/src/main/java/com/contentgrid/thunx/predicates/model/FunctionExpression.java

vierbergenlars · 2023-07-11T08:35:41Z

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

@@ -57,6 +63,7 @@ public ThunkExpression<Boolean> convert(Query query) {

        var expressions = query.stream()
                .map(this::convert)
+                .filter(Objects::nonNull)


Are there ever null values returned from this::convert?

I think that would indicate a bug, so filtering them out might be counterproductive, and actually result in an expression that can't be converted being dropped, thereby widening the access policies, which is an undesired effect.

We should prefer erroring out (even if it's a null pointer exception) over accidentally allowing access to data that should have been protected.

This filtering is done because there is an ability to return empty Term for in operation.
Example:
"index": 4, "terms": [ { "type": "ref", "value": [ { "type": "var", "value": "internal" }, { "type": "string", "value": "member_2" } ] }, { "type": "ref", "value": [ { "type": "var", "value": "input" }, { "type": "string", "value": "entity" }, { "type": "string", "value": "accountId" } ] }, { "type": "array", "value": [] # will return null in conversion, cause we need to skip empty in clause } ] }

That's why in this place we are filtering empty in cases

vierbergenlars · 2023-07-11T08:42:20Z

thunx-model/src/main/java/com/contentgrid/thunx/predicates/model/ThunkExpressionVisitor.java

@@ -6,5 +6,8 @@ public interface ThunkExpressionVisitor<T, C> {
    T visit(FunctionExpression<?> functionExpression, C context);
    T visit(SymbolicReference symbolicReference, C context);
    T visit(Variable variable, C context);
+    default T visit(CollectionValue collection, C context) {
+        throw new UnsupportedOperationException("Visit for CollectionValue is not yet implemented.");


During development/experimenting this is fine, but we should remove the default implementation.

At some, all visitors need to implement this method, so we can be sure that we can handle collection values.

One of the important ones that is still missing an implementation is the ExpressionJsonConverter.JsonEncoderVisitor, without which we can't send the partial expression to the application.

...rydsl/src/main/java/com/contentgrid/thunx/predicates/querydsl/QueryDslConvertingVisitor.java

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

vierbergenlars · 2023-07-11T09:00:25Z

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

+
+        @Override
+        public ThunkExpression<?> visit(ArrayTerm arrayTerm) {
+            arrayTerm.getValue().forEach(scalarTerm -> scalarTerm.accept(this));


Doing this would unpack a rego array ['a', 'b', ['c', 'd']] into ['a', 'b', 'c', 'd'], which is not a valid conversion.

Instead, this would probably need to use the same code as used above to collect scalars, and add the whole resulting CollectionValue as a scalar value.

Now SetTerm and ArrayTerm extends Term, that means that SetTerm cannot have SetTerm as a value. If you want to have such case when SetTerm contains SetTerm, then we need to change opa-client, to make SetTerm and ArrayTerm extend ScalarTerm

I've added support of collection in collection for opa-client (xenit-eu/opa-java-client#28) and for thunx

vierbergenlars · 2023-07-11T09:01:59Z

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

+
+        @Override
+        public ThunkExpression<?> visit(SetTerm setTerm) {
+            setTerm.getValue().forEach(scalarTerm -> scalarTerm.accept(this));


Same remark as for the array case, nested sets shouldn't be flattened.

vierbergenlars · 2023-07-11T09:09:41Z

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

+
+        @Override
+        public ThunkExpression<?> visit(QuerySet querySet) {
+            return null;


All these rego types are unexpected in a scalar collection, so they should throw an exception instead of silently dropping the term.

For example, in rego, you can have a Ref in an array, for example, when constructing input.entity.something in [input.entity.optionA, input.entity.optionB]

That is probably (I didn't check) a valid rego expression, but is not supported as entry in the current CollectionValue.

It's fine to limit ourselves to only scalar values for now, but we should not drop those terms silently.

I've added support of collection in collection for opa-client (xenit-eu/opa-java-client#28) and for thunx

ystrict · 2023-07-12T10:09:00Z

because on local using publishToMavenLocal it works good.

I found a compile error here: https://github.com/xenit-eu/thunx/pull/110/files#diff-e1519f085982147798370b05f00c49957d1c5f2be3db626be759193e055631a3R155

It's trying to pass Rego objects to something that should be a thunx Scalar.

It's strange that it compiles for you locally, do you still have some uncommitted changes, or perhaps an includeBuild() left in settings.gradle that is replacing some dependencies?

includeBuild is commented, but everything builds fine without any errors and it also works. Really strange, where did you find this error? It's trying to pass Rego objects to something that should be a thunx Scalar.

vierbergenlars · 2023-07-12T11:59:17Z

It's trying to pass Rego objects to something that should be a thunx Scalar.

This line:

Scalar.of(
   Set.of(new Term.Numeric(4), new Term.Numeric(5))
)

Scalar.of() accepts Collection<Scalar<?>>, but what you are creating is Set<Term.Numeric>. Term.Numeric comes from the opa-java-client, and Scalar is defined in thunx-model.

These types are not compatible with each other: Term.Numeric (the rego object) does not extend or implement Scalar<?> (the thunx model)

ystrict · 2023-07-12T12:03:39Z

It's trying to pass Rego objects to something that should be a thunx Scalar.

This line:
Scalar.of(
   Set.of(new Term.Numeric(4), new Term.Numeric(5))
)
Scalar.of() accepts Collection<Scalar<?>>, but what you are creating is Set<Term.Numeric>. Term.Numeric comes from the opa-java-client, and Scalar is defined in thunx-model.

These types are not compatible with each other: Term.Numeric (the rego object) does not extend or implement Scalar<?> (the thunx model)

thanks, let me check

ystrict · 2023-07-21T12:19:57Z

It's trying to pass Rego objects to something that should be a thunx Scalar.

This line:
Scalar.of(
   Set.of(new Term.Numeric(4), new Term.Numeric(5))
)
Scalar.of() accepts Collection<Scalar<?>>, but what you are creating is Set<Term.Numeric>. Term.Numeric comes from the opa-java-client, and Scalar is defined in thunx-model.

These types are not compatible with each other: Term.Numeric (the rego object) does not extend or implement Scalar<?> (the thunx model)

Hi, I've added json functionality, please take a look as soon as you have time

ystrict · 2023-08-16T12:29:55Z

Hi, my opa-client PR(xenit-eu/opa-java-client#28) was merged, please take a look on this one.

...ding-json/src/main/java/com/contentgrid/thunx/encoding/json/CollectionValueDeserializer.java

vierbergenlars · 2023-08-16T14:11:14Z

...encoding-json/src/main/java/com/contentgrid/thunx/encoding/json/ExpressionJsonConverter.java

    }

+    private static JsonScalarDto<?> processCollectionDto(CollectionValue collectionValue) {
+        Collection<JsonExpressionDto> result;
+        if (JsonCollectionValueDto.getTypeByClass(collectionValue.getResultType()).equals("set")) {


If you need to do typechecks here, this indicates that we need two separate visit methods: one for the set type and one for the array type.

This also means that we need two separate classes: CollectionValue needs to be split up in ArrayCollectionValue and SetCollectionValue. You can keep CollectionValue as an interface so it can be used in places where an array or a set would be sufficient.

Yeap, I agree with you, but can we merge this PR and create issue, for this? I will pick it up and fix in next PR, cause this one became to big and too time consuming. This change requires rewriting the whole PR:(

thunx-model/src/main/java/com/contentgrid/thunx/predicates/model/Scalar.java

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java

vierbergenlars · 2023-08-17T07:30:39Z

@ystrict: We are working on upgrading Thunx to Spring Boot 3, which will make it incompatible with Spring Boot 2 applications.

Please let us know if you have to integrate this in a Spring Boot 2 application, so we can plan a final release for Spring Boot 2 containing this PR.

ystrict · 2023-08-18T08:29:40Z

@ystrict: We are working on upgrading Thunx to Spring Boot 3, which will make it incompatible with Spring Boot 2 applications.

Please let us know if you have to integrate this in a Spring Boot 2 application, so we can plan a final release for Spring Boot 2 containing this PR.

Can you please include my changes to Spring Boot 2 release?

sonarcloud · 2023-12-19T10:49:00Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

thijslemmens · 2024-11-06T11:26:51Z

Really sorry to close this PR, but at this point we cannot get it merged in. Sorry @ystrict

ystrict added 2 commits June 19, 2023 16:30

added array, set and "IN" support

32e5d0c

Merge remote-tracking branch 'origin/main'

2c1598b

ystrict requested a review from a team as a code owner June 19, 2023 13:36

ystrict requested review from tgeens and rschev June 19, 2023 13:36

tgeens reviewed Jun 20, 2023

View reviewed changes

...-pdp-opa/src/main/java/com/contentgrid/thunx/pdp/opa/QuerySetToThunkExpressionConverter.java Show resolved Hide resolved

Added test and type optimizattion

ec25dd0

Merge branch 'main' into main

dcba01b

tgeens reviewed Jun 27, 2023

View reviewed changes

thunx-model/src/main/java/com/contentgrid/thunx/predicates/model/CollectionValue.java Outdated Show resolved Hide resolved

ystrict added 2 commits June 27, 2023 12:43

updated opa-client version

4df46bb

Merge remote-tracking branch 'origin/main'

caa4722

ystrict and others added 2 commits June 27, 2023 15:36

Changed CollectionValue data structure

d982522

Merge branch 'main' into main

76f4014

Merge branch 'main' into main

f564fdd

ystrict mentioned this pull request Jul 7, 2023

ThunX converter doesn't support Array, Set data structures and IN operator from future.keywords.in #119

Open

Merge branch 'main' into main

348b0ab

vierbergenlars reviewed Jul 11, 2023

View reviewed changes

Merge branch 'main' into main

403c28c

ystrict added 2 commits July 12, 2023 17:01

added collection in collection support

d724d4b

Added json functionality for CollectionValue

e2eac84

ystrict and others added 2 commits July 24, 2023 12:33

test fix

7343f85

Merge branch 'main' into main

d669080

vierbergenlars reviewed Aug 16, 2023

View reviewed changes

ystrict added 2 commits August 18, 2023 11:27

fixed comments

ca1996c

Merge remote-tracking branch 'origin/main'

8db0765

Merge branch 'main' into main

da66feb

thijslemmens closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Array, Set and operator 'IN' #110

Added Array, Set and operator 'IN' #110

ystrict commented Jun 19, 2023

tgeens commented Jun 21, 2023

ystrict commented Jun 23, 2023

tgeens commented Jun 26, 2023

ystrict commented Jun 26, 2023

ystrict commented Jun 26, 2023

ystrict commented Jun 26, 2023

tgeens commented Jun 27, 2023

ystrict commented Jun 27, 2023

tgeens commented Jun 27, 2023

ystrict commented Jul 3, 2023

ystrict commented Jul 11, 2023

vierbergenlars commented Jul 11, 2023

vierbergenlars Jul 11, 2023

ystrict Jul 12, 2023

ystrict Jul 12, 2023

vierbergenlars Jul 11, 2023

vierbergenlars Jul 11, 2023

ystrict Jul 12, 2023

ystrict Jul 12, 2023

vierbergenlars Jul 11, 2023

ystrict Jul 12, 2023

vierbergenlars Jul 11, 2023

ystrict Jul 12, 2023

ystrict commented Jul 12, 2023

vierbergenlars commented Jul 12, 2023

ystrict commented Jul 12, 2023

ystrict commented Jul 21, 2023

ystrict commented Aug 16, 2023

vierbergenlars Aug 16, 2023

ystrict Aug 18, 2023

vierbergenlars commented Aug 17, 2023

ystrict commented Aug 18, 2023

sonarcloud bot commented Dec 19, 2023

thijslemmens commented Nov 6, 2024

Added Array, Set and operator 'IN' #110

Added Array, Set and operator 'IN' #110

Conversation

ystrict commented Jun 19, 2023

tgeens commented Jun 21, 2023

ystrict commented Jun 23, 2023

tgeens commented Jun 26, 2023

ystrict commented Jun 26, 2023

ystrict commented Jun 26, 2023

ystrict commented Jun 26, 2023

tgeens commented Jun 27, 2023

ystrict commented Jun 27, 2023

tgeens commented Jun 27, 2023

ystrict commented Jul 3, 2023

ystrict commented Jul 11, 2023

vierbergenlars commented Jul 11, 2023

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

ystrict commented Jul 12, 2023

vierbergenlars commented Jul 12, 2023

ystrict commented Jul 12, 2023

ystrict commented Jul 21, 2023

ystrict commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vierbergenlars commented Aug 17, 2023

ystrict commented Aug 18, 2023

sonarcloud bot commented Dec 19, 2023

Quality Gate passed

thijslemmens commented Nov 6, 2024