Markings by jalphonso · Pull Request #3469 · NationalSecurityAgency/datawave

jalphonso · 2026-03-23T17:25:34Z

No description provided.

This reverts commit 1158ca1.

drewfarris · 2026-05-13T19:21:09Z

-    public void addColumn(String columnName, TypedValue columnTypedValue, Map<String,String> markings, String columnVisibility, Long timestamp) {
+    public void addColumn(String columnName, TypedValue columnTypedValue, Markings<?> markings, String columnVisibility, Long timestamp) {

        columnName = columnName.replaceAll(" ", "_");


I know this isn't part of your change set, but consider the fact that for single character strings, the following is generally more performant because the character to replace isn't treated as a regular expression. Also, generally I prefer to assign it to a new immutable variable instead of reusing a variable name, the impact to the stack is minimal.

Suggested change

columnName = columnName.replaceAll(" ", "_");

final String cleanColumnName = columnName.replace(' ', '_');

There's a second replaceAll on single characters later in this method as well

drewfarris · 2026-05-13T19:39:58Z

+        if (null == this.markings) {
+            this.markings = markings;
+        } else {
+            // if new markings are the same as the old markings, skip all of this
+            // they are the same and the markings value has already been validated
+            if (!this.markings.equals(markings)) {
+                // incoming markings are null so skip remaining logic
+                if (null != markings) {
+                    // we have existing markings, but they aren't valid so we update if the new ones are valid
+                    if (this.markings.isEmpty() && !markings.isEmpty()) {
                        this.markings = markings;
+                    } else {
+                        // otherwise combine
+                        try {
+                            Set<ColumnVisibility> columnVisibilities = Sets.newHashSet();
+                            columnVisibilities.add(this.markings.toColumnVisibility());
+                            columnVisibilities.add(markings.toColumnVisibility());
+                            ColumnVisibility combinedVisibility = markingFunctions.combineVisibilities(columnVisibilities);
+
+                            // use combined marking as new markings
+                            this.markings = markingFunctions.translateFromColumnVisibility(combinedVisibility);
+                        } catch (MarkingFunctions.Exception e) {
+                            LOGGER.error("Invalid markings {} skipping column {} = {}", markings, columnName, columnTypedValue, e);
+                            return;
+                        }
                    }


Perhaps something like the following instead?

try { this.markings = markingFunctions.combine(this.markings, markings); } catch (MarkingFunctions.Exception e) { LOGGER.error("Invalid markings {} skipping column {} = {}", markings, cleanColumnName, columnTypedValue, e); return; }

drewfarris · 2026-05-13T19:46:55Z

        request.setFieldValue(fieldValue);
        Map<String,String> markings = new HashMap<>();
-        markings.put(MarkingFunctions.Default.COLUMN_VISIBILITY, columnVisibility);
+        markings.put("columnVisibility", columnVisibility);


Suggested change

markings.put("columnVisibility", columnVisibility);

markings.put(AccessExpressionMarkings.COLUMN_VISIBILITY_KEY, columnVisibility);

drewfarris · 2026-05-13T19:56:26Z

+        public AccessExpressionMarkings combine(Markings<?> markings1, Markings<?> markings2) {
+            if (markings2 != null) {
+                if (markings1 == null) {
+                    markings1 = markings2;
+                } else {
+                    if (markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) {
+                        AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1;
+                        AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2;
+                        return combine(List.of(aem1, aem2));
+                    } else {
+                        throw new RuntimeException(
+                                        String.format("Unknown markings class %s or %s", markings1.getClass().getName(), markings2.getClass().getName()));
+                    }
+                }
+            }
+            return markings1 == null ? null : (AccessExpressionMarkings) markings1;


Consider something a little less complex? Still, not a fan of the potential ClassCastExceptions here, so this suggestion might need more thought if we want to avoid or handle those more cleanly.

Suggested change

public AccessExpressionMarkings combine(Markings<?> markings1, Markings<?> markings2) {

if (markings2 != null) {

if (markings1 == null) {

markings1 = markings2;

} else {

if (markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) {

AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1;

AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2;

return combine(List.of(aem1, aem2));

} else {

throw new RuntimeException(

String.format("Unknown markings class %s or %s", markings1.getClass().getName(), markings2.getClass().getName()));

}

}

}

return markings1 == null ? null : (AccessExpressionMarkings) markings1;

public AccessExpressionMarkings combine(Markings<?> markings1, Markings<?> markings2) {

if (markings2 == null) {

return (AccessExpressionMarkings) markings1;

}

if (markings1 == null) {

return (AccessExpressionMarkings) markings2;

}

if (markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) {

AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1;

AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2;

return combine(List.of(aem1, aem2));

}

throw new RuntimeException(String.format("Unknown markings class %s or %s", markings1.getClass().getName(), markings2.getClass().getName()));

drewfarris · 2026-05-13T19:59:53Z

Nice coverage, but it would be cool to see a few more unhappy-path tests here (e.g., mismatched parenthesis, illegal operators like && and || or + or ,)

drewfarris · 2026-05-13T20:02:05Z

Likewise, maybe a couple more bizarre tests here like A&A|B&B|(A&A&A) or (A|B|C)&(B|C|D) can normalize clean things like this up?

drewfarris · 2026-05-13T20:07:35Z

    private static final Logger log = Logger.getLogger(FrequencyMetadataAggregator.class);
    private static final String NULL_BYTE = "\0";
-    private static final MarkingFunctions markingFunctions = MarkingFunctions.Factory.createMarkingFunctions();
+    private final MarkingFunctions<?> markingFunctions;


I'm wondering if the prevalence of MarkingFunctions<?> and other wildcard generic types throughout this PR is suggesting that we should consider just using plain sub-classing instead of generics the code. Replacing the <?> everywhere it's used it going to be very difficult, but maybe that's not the end goal.

drewfarris · 2026-05-14T12:50:29Z

Consider adding a direct dependency to accumulo-utils in the dictionary api pom?

drewfarris · 2026-05-14T12:51:28Z

+            Markings<?> currMarkings = columnMarkingsMap.get(columnName);

-            if (currMarkings.equals(markings) == false) {
+            if (!currMarkings.equals(markings)) {


Consider a null check here?

drewfarris · 2026-05-14T12:53:30Z

                for (Map.Entry<String,String> entry : combinedColumnMarkings.entrySet()) {
                    String columnName = entry.getKey();
                    String combinedString = entry.getValue();
                    int x = combinedString.lastIndexOf(':');


Consider whether the combined string might have colons in it from the values created in CacheableQueryRowImpl where we encode `accessExpression + ':' + columnVisibility' and this get this split incorrect.

drewfarris · 2026-05-14T13:07:01Z

+@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
+public interface Markings<T> {
+
+    T getMarkings();


Consider whether we can just drop this method entirely from the interface definition and just remove the generic type on Markings.

jschmidt10 · 2026-05-14T14:01:55Z

+
+    @Test
+    public void testJaxbMarshal() throws Exception {
+        JAXBContext ctx = JAXBContext.newInstance(AccessExpressionMarkings.class);


You could move all the JAXB boilerplate into a method:

AccessExpressionMarkings original = // ... test setup AccessExpressionMarkings copy = makeJaxbRoundtrip(original); // assertions

jschmidt10 · 2026-05-14T14:03:07Z

+    public void testProtostuffRoundTripEmpty() {
+        AccessExpressionMarkings original = AccessExpressionMarkings.builder().accessExpression(null).build();
+
+        LinkedBuffer buffer = LinkedBuffer.allocate();


Same comment as JAXB roundtrip boilerplate.

jschmidt10 · 2026-05-14T14:06:47Z

+        <dependency>
+            <groupId>org.projectlombok</groupId>
+            <artifactId>lombok</artifactId>
+            <version>1.18.36</version>


Can we bubble these versions up to the top level parent and just inherit here?

jschmidt10 · 2026-05-14T14:09:16Z

        <datawave.webservice.namespace>http://webservice.datawave.nsa/v1</datawave.webservice.namespace>
        <version.commons-codec>1.17.1</version.commons-codec>
-        <version.datawave>7.33.1</version.datawave>
+        <version.datawave>7.39.0-SNAPSHOT</version.datawave>


Do you have changes in other repos that we'll need before we can merge this?

jschmidt10 · 2026-05-14T14:12:01Z

 import io.protostuff.Message;
 import io.protostuff.Output;
 import io.protostuff.Schema;
+import lombok.Setter;


Is this the only Lombok addition? If the plan is to use it across the board, I'm cool but if this is the only usage, I'd suggest we drop the dependency and leave the original setter.

jschmidt10 · 2026-05-15T12:01:13Z

        DESC desc = this.responseObjectFactory.getDescription();
-        Map<String,String> markings = Maps.newHashMap();
-        markings.put("columnVisibility", columnVisibility);
+        Markings<?> markings = AccessExpressionMarkings.builder().accessExpression(ACCESS.newExpression(columnVisibility)).build();


It looks like a large portion of AccessExpressionMarkings are being built from columnVisibilities. Thoughts on adding that to the builder?

AccessExpressionMarkings.builder().colvis(columnVisibility).build();

jschmidt10 · 2026-05-15T12:05:10Z

        Multimap<Map.Entry<String,String>,DefaultDescription> descriptions = HashMultimap.create();
-        descriptions.put(new AbstractMap.SimpleEntry<>("fooField", "fooType"), new DefaultDescription("my foo field", markings));
-        descriptions.put(new AbstractMap.SimpleEntry<>("barField", "barType"), new DefaultDescription("my bar field", markings));
+        descriptions.put(new AbstractMap.SimpleEntry<>("fooField", "fooType"), new DefaultDescription("my foo field", AccessExpressionMarkings.create("USER")));


Not your code but a nice improvement over the entry creation is Map.entry(key, value) instead of the inner SimpleEntry thing here.

avgAGB · 2026-05-14T23:51:30Z

+                    String oldColViz = AccessExpressionUtil.normalize(AccessExpressionUtil.toAccessExpression(oldColumnVisibility)).getExpression();
+                    String thisVis = AccessExpressionUtil.normalize(AccessExpressionUtil.toAccessExpression(thisViz)).getExpression();
                    if (!oldColViz.equals(thisVis)) {
                        log.trace("Skipping key that does not match with column visibility: {}", e.getKey());


small touch, but could check if trace is enabled before the log statement. also on line 880

jalphonso self-assigned this Mar 23, 2026

jalphonso marked this pull request as draft March 23, 2026 17:25

jalphonso force-pushed the accumulo-access branch 8 times, most recently from e86e7ba to 0e0b4d2 Compare March 26, 2026 01:35

ddanielr reviewed Mar 26, 2026

View reviewed changes

Comment thread core/base-rest-responses/src/main/java/datawave/marking/ColumnVisibilitySecurityMarking.java Outdated

jalphonso force-pushed the accumulo-access branch 3 times, most recently from 07765cd to 65db1ad Compare March 27, 2026 17:27

keith-turner reviewed Mar 27, 2026

View reviewed changes

jalphonso force-pushed the accumulo-access branch 5 times, most recently from 8bc250c to 98f8567 Compare April 2, 2026 17:24

jalphonso force-pushed the accumulo-access branch from 98f8567 to b07ead2 Compare April 6, 2026 21:01

jsduncan2 reviewed Apr 7, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionUtil.java Outdated

jalphonso force-pushed the accumulo-access branch 2 times, most recently from cceab28 to e72fd54 Compare April 7, 2026 17:48

jsduncan2 reviewed Apr 8, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionUtil.java Outdated

jsduncan2 reviewed Apr 8, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionUtil.java Outdated

jsduncan2 reviewed Apr 8, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionUtil.java

jsduncan2 reviewed Apr 8, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/Markings.java

jsduncan2 reviewed Apr 9, 2026

View reviewed changes

Comment thread core/base-rest-responses/src/main/java/datawave/webservice/query/result/event/HasMarkings.java

jschmidt10 reviewed Apr 9, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/MarkingFunctions.java

jalphonso force-pushed the accumulo-access branch from 691ba20 to 4bf29f6 Compare May 12, 2026 18:32

Joe Alphonso added 2 commits May 12, 2026 15:04

remove snapshots for unaffected modules

8e5fda5

remove extraneous test change

c7ca57a

jalphonso force-pushed the accumulo-access branch from 4bf29f6 to 6076aad Compare May 12, 2026 19:16

jalphonso marked this pull request as ready for review May 12, 2026 19:43

use -JFA-20260512-1 modifier in POMs

1158ca1

jalphonso force-pushed the accumulo-access branch from 6076aad to 1158ca1 Compare May 12, 2026 20:42

jschmidt10 reviewed May 13, 2026

View reviewed changes

Comment thread core/cached-results/src/main/java/datawave/core/query/cachedresults/CacheableQueryRowImpl.java Outdated

jschmidt10 reviewed May 13, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionMarkings.java Outdated

jschmidt10 reviewed May 13, 2026

View reviewed changes

Comment thread core/utils/accumulo-utils/src/main/java/datawave/marking/AccessExpressionMarkings.java

jschmidt10 reviewed May 13, 2026

View reviewed changes

Comment thread core/modification/src/main/java/datawave/modification/MutableMetadataHandler.java

fix spotbug issue

49540a8

FineAndDandy reviewed May 13, 2026

View reviewed changes

Comment thread core/base-rest-responses/src/main/java/datawave/marking/ColumnVisibilitySecurityMarking.java

Jeff MR feedback round 1

7956b99

jalphonso force-pushed the accumulo-access branch from 1c454a1 to 7956b99 Compare May 13, 2026 15:33

Revert "use -JFA-20260512-1 modifier in POMs"

c21f3d9

This reverts commit 1158ca1.

drewfarris reviewed May 13, 2026

View reviewed changes

drewfarris reviewed May 14, 2026

View reviewed changes

jschmidt10 reviewed May 14, 2026

View reviewed changes

Joe Alphonso added 2 commits May 14, 2026 12:27

fix XmlElement usage in DefaultEvent and DefaultField for markings

c07701b

Fix: Handle null ColumnVisibility in ModelKeyParser

9065fc8

jschmidt10 reviewed May 15, 2026

View reviewed changes

Joe Alphonso added 2 commits May 15, 2026 10:08

Fix: Handle null/empty ColumnVisibility in MetadataDescriptionsHelper

7be1fe8

update accumulo-access-core version

dc20c60

avgAGB reviewed May 15, 2026

View reviewed changes

	columnName = columnName.replaceAll(" ", "_");
	final String cleanColumnName = columnName.replace(' ', '_');

	markings.put("columnVisibility", columnVisibility);
	markings.put(AccessExpressionMarkings.COLUMN_VISIBILITY_KEY, columnVisibility);

Conversation

jalphonso commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants