Skip to content

Using DataSet view for Builders #1

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.

Sign up for GitHub

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

Open
ankit27755 wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Using DataSet view for Builders #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ad523fd
Added an optimistic version of safeOp(). Only to be used for supplier…
r0goyal Apr 14, 2025
6055291
Added concept of data set view
r0goyal Apr 14, 2025
33c9635
Merge branch 'cpu_optimization_one' into data_set_view
r0goyal Apr 15, 2025
b183533
Merge branch 'cpu_optimization_one' into data_set_view
r0goyal Apr 15, 2025
f133db5
Reverted DataBuilderMeta changes
r0goyal Apr 15, 2025
0da7972
Reverted DataBuilderMeta changes
r0goyal Apr 15, 2025
95c0103
Reverted safeOp() related changes
r0goyal Apr 15, 2025
4b08885
Reverted safeOp() related changes
r0goyal Apr 15, 2025
9404a37
Reverted to TreeMap()
r0goyal Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions src/main/java/io/appform/databuilderframework/engine/DataBuilderContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,24 +33,6 @@ public DataBuilderContext() {
this.contextData = contextData;
}

/**
* Get only the accessible data for this builder.
*
* @param builder The builder that wants to access this data.
* @return A dataset that contains only the data accessible to the builder.
* @deprecated This method will be removed in the near future. Do not use this in newer projects.
* Reasoning: This is redundant given the set sent to a builder is already filtered
*/
@Deprecated
public DataSet getDataSet(DataBuilder builder) {
Preconditions.checkNotNull(builder.getDataBuilderMeta(), "No metadata present in this builder");
val allowed = Utils.sanitize(builder.getDataBuilderMeta().getAccessibleDataSet());
if (allowed.isEmpty()) {
return new DataSet(Collections.emptyMap());
}
return new DataSet(dataSet.filter(allowed));
}

/**
* Access the data set. Ideally a builder should only access data as declared.
*
Expand All @@ -60,10 +42,6 @@ public DataSet getDataSet() {
return dataSet;
}

public void setDataSet(DataSet dataSet) {
this.dataSet = dataSet;
}

/**
* Some data to be saved in the context. This data is read-only from inside the builder.
*
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/io/appform/databuilderframework/engine/DataSetAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io.appform.databuilderframework.model.Data;
import io.appform.databuilderframework.model.DataDelta;
import io.appform.databuilderframework.model.DataSet;
import io.appform.databuilderframework.model.DataSetView;
import lombok.val;

import java.util.Collections;
Expand Down Expand Up @@ -37,8 +38,8 @@ public <T extends Data> T get(Class<T> tClass) {
public <T extends Data> T get(String key, Class<T> tClass) {
val data = dataSet.get(key);
return null == data
? null
: tClass.cast(data);
? null
: tClass.cast(data);
}

/**
Expand Down Expand Up @@ -66,7 +67,7 @@ public <B extends DataBuilder, T extends Data> T getAccessibleData(String key, B
* @return
*/
public DataSet getAccesibleDataSetFor(DataBuilder builder) {
return new DataSet(dataSet.filter(builder.getDataBuilderMeta().getAccessibleDataSet()));
return new DataSetView(dataSet, builder.getDataBuilderMeta().getAccessibleDataSet());
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/appform/databuilderframework/engine/SimpleDataFlowExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.TreeMap;
import java.util.function.Supplier;
Expand Down Expand Up @@ -38,7 +39,7 @@ protected DataExecutionResponse run(
DataBuilderFactory builderFactory) throws DataBuilderFrameworkException, DataValidationException {
val executionGraph = dataFlow.getExecutionGraph();
val dataSet = dataFlowInstance.getDataSet().accessor().copy(); //Create own copy to work with
val dataSetAccessor = DataSet.accessor(dataSet);
val dataSetAccessor = dataSet.accessor();
val responseData = new TreeMap<String, Data>();
val activeDataSet = new HashSet<String>();
val dependencyHierarchy = executionGraph.getDependencyHierarchy();
Expand Down
14 changes: 7 additions & 7 deletions src/main/java/io/appform/databuilderframework/model/DataBuilderMeta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public class DataBuilderMeta implements Comparable<DataBuilderMeta>, Serializabl
private String name;

private int rank;


/**
* Set of {@link io.appform.databuilderframework.model.Data} this {@link io.appform.databuilderframework.engine.DataBuilder}
* can consume optionally, i.e. this {@link io.appform.databuilderframework.model.Data}
Expand All @@ -70,7 +70,7 @@ public DataBuilderMeta(Set<String> consumes, String produces, String name) {
}

@Builder
public DataBuilderMeta(Set<String> consumes, String produces, String name,
public DataBuilderMeta(Set<String> consumes, String produces, String name,
Set<String> optionals, Set<String> access) {
this.consumes = consumes;
this.produces = produces;
Expand All @@ -79,7 +79,7 @@ public DataBuilderMeta(Set<String> consumes, String produces, String name,
this.access = access;
}


public DataBuilderMeta() {
}

Expand All @@ -91,7 +91,7 @@ public Set<String> getEffectiveConsumes(){
return consumes;
}
}

@JsonIgnore
public Set<String> getAccessibleDataSet(){
Set<String> output = consumes;
Expand All @@ -103,7 +103,7 @@ public Set<String> getAccessibleDataSet(){
}
return output;
}

public int compareTo(DataBuilderMeta rhs) {
return name.compareTo(rhs.getName());
}
Expand All @@ -113,4 +113,4 @@ public DataBuilderMeta deepCopy() {
Set<String> accessCopy = (access != null) ? ImmutableSet.copyOf(access) : null;
return new DataBuilderMeta(ImmutableSet.copyOf(consumes), produces, name, optionalCopy, accessCopy);
}
}
}
11 changes: 3 additions & 8 deletions src/main/java/io/appform/databuilderframework/model/DataSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
import org.hibernate.validator.constraints.NotEmpty;

import javax.validation.constraints.NotNull;
import java.util.*;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.StampedLock;
import java.util.function.Supplier;
Expand Down Expand Up @@ -64,13 +66,6 @@ public <T extends Data> DataSet add(T data) {
return add(Utils.name(data.getClass()), data);
}

public Map<String, Data> filter(final Collection<String> requiredKeys) {
if (null == requiredKeys || requiredKeys.isEmpty()) {
return Collections.emptyMap();
}
return safeOp(() -> Maps.filterKeys(Utils.sanitize(availableData), Predicates.in(requiredKeys)));
}

public Data get(final String name) {
if (Strings.isNullOrEmpty(name)) {
return null;
Expand Down
72 changes: 72 additions & 0 deletions src/main/java/io/appform/databuilderframework/model/DataSetView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package io.appform.databuilderframework.model;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Predicates;
import com.google.common.base.Strings;
import com.google.common.collect.Maps;
import io.appform.databuilderframework.engine.DataSetAccessor;
import io.appform.databuilderframework.engine.Utils;
import lombok.val;
import org.hibernate.validator.constraints.NotEmpty;

import javax.validation.constraints.NotNull;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.StampedLock;
import java.util.function.Supplier;

/**
* Working set for data
* Contains all the data that has been provided by the system as well as those generated by the system.
*/
public class DataSetView extends DataSet {

private final DataSet dataSet;
private final Set<String> allowedKeys;

public DataSetView(final DataSet dataSet,
final Set<String> allowedKeys) {
this.dataSet = dataSet;
this.allowedKeys = allowedKeys;
}

public DataSetView add(String dataName, Data data) {

@deputydev-agent deputydev-agent Bot Apr 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ERROR: The DataSetView class extends DataSet but doesn't properly implement or override several methods. It throws UnsupportedOperationException for methods like add(), filter(), and copyInto() but fails to annotate them with @OverRide. This makes the class hierarchy error-prone and can cause confusion about which methods are actually overriding parent methods versus just hiding them.

// Add @Override annotation to all methods that override parent methods
@Override
public DataSetView add(String dataName, Data data) {
    throw new UnsupportedOperationException();
}

@Override
public DataSetView add(final Collection<Data> data) {
    throw new UnsupportedOperationException();
}

@Override
public <T extends Data> DataSetView add(T data) {
    throw new UnsupportedOperationException();
}

@Override
public Map<String, Data> filter(final Collection<String> requiredKeys) {
    throw new UnsupportedOperationException();
}

@Override
public boolean containsAll(final Collection<String> requiredKeys) {
    // Implementation here
}

@Override
public void copyInto(final Map<String, Data> outMap, Collection<String> excludedKeys) {
    throw new UnsupportedOperationException();
}

throw new UnsupportedOperationException();
}

public DataSetView add(final Collection<Data> data) {
throw new UnsupportedOperationException();
}

public <T extends Data> DataSetView add(T data) {
throw new UnsupportedOperationException();
}

public Map<String, Data> filter(final Collection<String> requiredKeys) {
throw new UnsupportedOperationException();
}

public Data get(final String name) {
if (allowedKeys.contains(name)) {
return dataSet.get(name);
}
return null;
}

public boolean containsAll(final Collection<String> requiredKeys) {
// TODO Fix this. Should requiredKeys be filtered on allowedKeys ???

@deputydev-agent deputydev-agent Bot Apr 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SECURITY: The DataSetView implementation in src/main/java/io/appform/databuilderframework/model/DataSetView.java contains a TODO comment on line 60 that indicates a potential security vulnerability. The containsAll() method does not properly filter the requiredKeys against allowedKeys, which could allow a builder to check for the existence of keys it shouldn't have access to. This information disclosure vulnerability could be exploited if a malicious DataBuilder checks for existence of data it shouldn't be able to see.

public boolean containsAll(final Collection<String> requiredKeys) {
    // Filter requiredKeys to only include allowedKeys
    Set<String> filteredKeys = requiredKeys.stream()
        .filter(allowedKeys::contains)
        .collect(Collectors.toSet());
    return dataSet.containsAll(filteredKeys);
}

return dataSet.containsAll(requiredKeys);
}

public void copyInto(final Map<String, Data> outMap, Collection<String> excludedKeys) {
throw new UnsupportedOperationException();
}

public DataSetAccessor accessor() {
return accessor(this);
}

}

@deputydev-agent deputydev-agent Bot Apr 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CODE_MAINTAINABILTIY: The DataSetView implementation doesn't properly override all methods from DataSet, which could lead to unexpected behavior. Specifically, methods like deepCopy(), merge(), and other common operations are missing, which violates the Liskov Substitution Principle (LSP), a key SOLID principle.

// Additional methods to implement for full DataSet compatibility
public DataSet deepCopy() {
    return new DataSetView(dataSet.deepCopy(), new HashSet<>(allowedKeys));
}

@Override
public DataSet merge(DataDelta dataDelta) {
    throw new UnsupportedOperationException("DataSetView is read-only and does not support merge operations");
}

// Ensure all relevant DataSet methods are properly overridden with appropriate behavior

2 changes: 1 addition & 1 deletion src/test/java/examples/bloghomepagebuilder/controller/HomePageControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void testHomePageMTOpt() throws Exception {
private void runHomePageTest(DataFlowExecutor executor) throws Exception {
val request = new HomePageRequest("2321312312", "2323454", "Blah".getBytes());
val stopwatch = Stopwatch.createStarted();
for(long i = 0; i < 100_000; i++) {
for(long i = 0; i < 10000_000; i++) {

@deputydev-agent deputydev-agent Bot Apr 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • PERFORMANCE_OPTIMIZATION: The test loop iteration count has been drastically increased from 100,000 to 10,000,000 (a 100x increase). This change could lead to excessively long test execution times and potential resource exhaustion, especially in resource-constrained CI/CD environments. It's important to ensure that regression tests remain efficient to provide timely feedback during development.
for(long i = 0; i < 100_000; i++) {
    HomePageResponse response = executor.run(homePageDataFlow, request).get(HomePageResponse.class);
    Assert.assertNotNull(response);
    //System.out.println(new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(response));
}

HomePageResponse response = executor.run(homePageDataFlow, request).get(HomePageResponse.class);
Assert.assertNotNull(response);
//System.out.println(new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(response));
Expand Down