Skip to content
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

Jump to bottom

ApplyViewToElementsFunction does not apply Schema validation #3059

Open
t92549 opened this issue Nov 2, 2023 · 4 comments · May be fixed by #3060
Open

ApplyViewToElementsFunction does not apply Schema validation #3059

t92549 opened this issue Nov 2, 2023 · 4 comments · May be fixed by #3060
Assignees
@GCHQDev404
Labels
bug Confirmed or suspected bug
Milestone
Medium Term Backlog

Comments

@t92549
Copy link
Contributor

t92549 commented Nov 2, 2023

Describe the bug
ApplyViewToElementsFunction will apply the View of an operation to the results from multiple subgraphs. This is meant to ensure that the results are correctly aggregated and validated after a query to a FederatedStore.

However, despite the View validation being applied, the Schema validation is not applied. This could be intended, but I suspect it is not, as the intention of this merge function is to ensure all aggregation and validation happens as if the results came from one graph. If this is indeed unintended, I would suggest the merge function should be renamed to something like ValidateAndAggregateElements.

Additional context
The reason this happens is that the results are currently merged inside a MapStore and the MapStore does not currently support schema validation: #2613.

Some potential fixes are:

  • Add MapStore validation as suggested in Add validation to Map Store #2613.
  • Change the ApplyViewToElementsFunction to create an ephemeral AccumuloStore rather than MapStore
@t92549 t92549 added the bug Confirmed or suspected bug label Nov 2, 2023
@GCHQDev404
Copy link
Contributor

GCHQDev404 commented Nov 2, 2023 •
edited

Test to pass here.

https://github.com/gchq/Gaffer/tree/gh-3059-federated-store-applyviewtoelements-schema-validation

@GCHQDev404
Copy link
Contributor

GCHQDev404 commented Nov 2, 2023 •
edited

Having the this tap into an existing instance of AccumuloStore for the temporary map is the way forward.
but this won't work with a GAAS situation with a FederatedStore which only has ProxiesStore pointed at a remote Gaffer graph.

@GCHQDev404
Copy link
Contributor

Technically the Function is called "ApplyView" to elements. which is does do. But I think it should be better and apply the Schema Validation.

GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
gh-3059 Updaed ApplyViewToElementsFunction to merge the Schema valida…
5d4b3e9 
…tion into view.
@github-actions github-actions bot linked a pull request Nov 3, 2023 that will close this issue
Open
GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
gh-3059 Updated ApplyViewToElementsFunction to merge the Schema valid…
b4d5b9d 
…ation into view POST Aggregation to work after a groupBy aggregation change.
GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
Merge remote-tracking branch 'origin/gh-3059-federated-store-applyvie…
644c8a5 
…wtoelements-schema-validation' into gh-3059-federated-store-applyviewtoelements-schema-validation

# Conflicts:
#	store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/util/ApplyViewToElementsFunction.java
GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
gh-3059 Updated ApplyViewToElementsFunction to merge the Schema valid…
a3f2597 
…ation should only be done for the TemporaryStore of type MapStore
GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
gh-3059 Nullpointers and Test cache clearing
11f406e
GCHQDev404 added a commit that referenced this issue Nov 3, 2023
@GCHQDev404
gh-3059 test fixes
bffc015
@GCHQDev404 GCHQDev404 self-assigned this Nov 3, 2023
@GCHQDev404
Copy link
Contributor

It seems the the Schema can be searched for any Group's Validation function and these can be used in a View post aggregate filter. The View is made if it doesn't exist or it merges if it does.

PostAggregate because this happens after the GroupBy aggregation.

GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 map improvements.
33a5721
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 stream improvements.
8e07f18
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 rename.
abd4950
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 Graph or GraphSerialisable.
7906716
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 test move.
cd31331
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 Other merge graphs options unsupported.
bae4d16
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 tidy up
a65e57e
GCHQDev404 added a commit that referenced this issue Nov 6, 2023
@GCHQDev404
gh-3059 tidy up
48942f7
@github-actions github-actions bot mentioned this issue Nov 6, 2023
Closed
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 logic typo
5dba0fe
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
Merge branch 'gh-3059-federated-store-applyviewtoelements-schema-vali…
1737257 
…dation' into gh-3059-other-temp-merge-graphs

# Conflicts:
#	store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/util/FederatedElementFunction.java
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
Merge branch 'develop' into gh-3059-federated-store-applyviewtoelemen…
df0ec86 
…ts-schema-validation
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 FederatedElementFunction merge graph is only supports MapStor…
2d5bbd4 
…e at the moment
@github-actions github-actions bot mentioned this issue Nov 7, 2023
Closed
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 Javadoc
20e7548
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 rename
ec966c3
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
Merge branch 'gh-3059-federated-store-applyviewtoelements-schema-vali…
8d8cd85 
…dation' into gh-3059-other-temp-merge-graphs

# Conflicts:
#	store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/util/FederatedStoreUtil.java
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 rename
613fb6b
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 rename
7924fa7
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 rename
9721d56
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
Merge branch 'gh-3059-federated-store-applyviewtoelements-schema-vali…
087c061 
…dation' into gh-3059-other-temp-merge-graphs
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 typo
9917c32
GCHQDev404 added a commit that referenced this issue Nov 7, 2023
@GCHQDev404
gh-3059 typo
d9f1c0c
GCHQDev404 added a commit that referenced this issue Nov 9, 2023
@GCHQDev404
gh-3059 GraphSerialisable is JSONSerialisable
3cee0d5
GCHQDev404 added a commit that referenced this issue Nov 9, 2023
@GCHQDev404
gh-3059 Specify a merge graph.
6a96fe5
GCHQDev404 added a commit that referenced this issue Nov 9, 2023
@GCHQDev404
gh-3059 Avoiding name collision will make it harder to teardown the c…
f37f2cb 
…orrect graph.
GCHQDev404 added a commit that referenced this issue Nov 9, 2023
@GCHQDev404
gh-3059 Avoiding name collision will make it harder to teardown the c…
20cdae3 
…orrect graph.
GCHQDev404 added a commit that referenced this issue Nov 14, 2023
@GCHQDev404
Merge branch 'develop' into gh-3059-federated-store-applyviewtoelemen…
1029e27 
…ts-schema-validation
GCHQDev404 added a commit that referenced this issue Nov 14, 2023
@GCHQDev404
gh-3059 merge conflict
d2ab3c0
GCHQDev404 added a commit that referenced this issue Feb 23, 2024
@GCHQDev404
gh-3059 merge conflict
6c7ee0b
GCHQDev404 added a commit that referenced this issue Feb 23, 2024
@GCHQDev404
Merge remote-tracking branch 'origin/develop' into gh-3059-federated-…
a2f369f 
…store-applyviewtoelements-schema-validation

# Conflicts:
#	store-implementation/federated-store/src/test/java/uk/gov/gchq/gaffer/federatedstore/FederatedStoreSchemaTest.java
GCHQDev404 added a commit that referenced this issue Feb 26, 2024
@GCHQDev404
gh-3059 PR change
f8a948a
GCHQDev404 added a commit that referenced this issue Feb 26, 2024
@GCHQDev404
gh-3059 PR change
5316093
This was referenced Feb 26, 2024
Closed
Merged
GCHQDev404 added a commit that referenced this issue Mar 12, 2024
@GCHQDev404
Merge remote-tracking branch 'origin/develop' into gh-3059-federated-…
d4c5e79 
…store-applyviewtoelements-schema-validation

# Conflicts:
#	store-implementation/federated-store/src/test/java/uk/gov/gchq/gaffer/federatedstore/operation/handler/impl/FederatedOperationChainHandlerTest.java
@GCHQDeveloper314 GCHQDeveloper314 added this to the Medium Term Backlog milestone Mar 15, 2024
GCHQDev404 added a commit that referenced this issue Mar 20, 2024
@GCHQDev404
Merge branch 'develop' into gh-3059-federated-store-applyviewtoelemen…
384c308 
…ts-schema-validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@GCHQDev404 GCHQDev404

Labels
bug Confirmed or suspected bug
Projects
None yet
Milestone
Medium Term Backlog
Development

Successfully merging a pull request may close this issue.

Gh-3059: ApplyViewToElementsFunction apply Schema validation gchq/Gaffer
3 participants
@GCHQDev404 @t92549 @GCHQDeveloper314