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.

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

Fix FederatedStore checks for DYNAMIC_SCHEMA #2580

Closed
t92549 opened this issue Jan 28, 2022 · 4 comments · Fixed by #2915
Closed

Fix FederatedStore checks for DYNAMIC_SCHEMA #2580

t92549 opened this issue Jan 28, 2022 · 4 comments · Fixed by #2915
Assignees
Labels
enhancement Improvement to existing functionality/feature federated-store Specific to/touches the federated-store module

Comments

@t92549
Copy link
Contributor

t92549 commented Jan 28, 2022

Due to a partial implementation of #2008, currently FederatedOperationChains only validate the View if the graph does not have the DYNAMIC_SCHEMA trait:


} else if (!graph.getStoreTraits().contains(StoreTrait.DYNAMIC_SCHEMA)) {

This trait only exists in the FederatedStore as its getTraits() implementation returns ALL_TRAITS. This effectively means that only FederatedStores and ProxyStores which point to a FederatedStore will not be validated here. However, this cannot be used going forward because:

  • FederatedStore fixes introduced in the FederatedStore alpha mean it will no longer return ALL_TRAITS
  • the getTraits function is being replaced by the Operation which behaves differently, instead of returning ALL_TRAITS it will use the FederatedGetTraitsHandler which correctly returns the traits that the FederatedStore's stores have

Therefore, before the getTraits/getStoreTraits function can be completely removed, this usage has to be replaced (under this ticket). This can be done in various ways:

  • deciding on a different solution to the partial implementation of Remove view Edge Entity Validation for operations #2008: either by fully implementing it or reverting it perhaps?
  • replacing the FederatedStore check with something else smarter than this DYNAMIC_SCHEMA trait check

A solution to this must be discussed, decided on and implemented before getTraits can be completely removed and it seems that it will be best to wait until the other FederatedStore changes first to do so.

@t92549 t92549 added the federated-store Specific to/touches the federated-store module label Jan 28, 2022
@t92549 t92549 added this to the v2.0.0-alpha-0.5 milestone Jan 28, 2022
@t92549
Copy link
Contributor Author

t92549 commented Jan 31, 2022

The fix for this has to be smarter than just checking if the FederatedStore is assignable from the store in question, because it can also be valid for ProxyStores pointing to FederatedStores

@lb324567
Copy link
Member

lb324567 commented Oct 26, 2022

Needs investigation to decide if this needs to stay in alpha5 or move to post V2. Might just result in code being deleted if it's not required

@GCHQDev404
Copy link
Contributor

This will link in with #2581

GCHQDev404 added a commit that referenced this issue Jan 25, 2023
@GCHQDev404 GCHQDev404 self-assigned this Jan 25, 2023
GCHQDev404 added a commit that referenced this issue Feb 21, 2023
@GCHQDev404 GCHQDev404 linked a pull request Feb 21, 2023 that will close this issue
GCHQDev404 added a commit that referenced this issue Feb 21, 2023
…chema-removal

# Conflicts:
#	store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/operation/FederatedOperationChainValidator.java
GCHQDev404 added a commit that referenced this issue Feb 21, 2023
GCHQDev404 added a commit that referenced this issue Feb 21, 2023
…t schema because Store.initialise() doesn't set original schema.
GCHQDev404 added a commit that referenced this issue Feb 21, 2023
…s compact schema because Store.initialise() doesn't set original schema.
@t92549 t92549 added the enhancement Improvement to existing functionality/feature label Feb 22, 2023
GCHQDeveloper314 added a commit that referenced this issue Mar 1, 2023
@GCHQDeveloper314 GCHQDeveloper314 linked a pull request Mar 1, 2023 that will close this issue
GCHQDeveloper314 added a commit that referenced this issue Mar 1, 2023
* Remove DYNAMIC_SCHEMA Trait and usages

* Re-enable disabled test in Federated Store
@GCHQDeveloper314
Copy link
Member

GCHQDeveloper314 commented Mar 1, 2023

Closed by #2915

The solution for this was to remove the getTraits method from Gaffer and use the operation instead, combined with removing the DYNAMIC_SCHEMA trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing functionality/feature federated-store Specific to/touches the federated-store module
Projects
None yet
4 participants