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

Gh-3018: Migrate GraphConfig code out of Graph #3050

Merged
merged 20 commits into from
Nov 10, 2023

Conversation

tb06904
Copy link
Member

@tb06904 tb06904 commented Oct 23, 2023

Fair bit of refactoring to the Graph and GraphConfig classes this lead to some tweaks to testing as it highlighted some bugs.

As an overview the following methods have been altered:

Graph.java

  • updateSchema() -> broken into applyParentSchemas() and loadSchemaFromJson() and refactored.
  • updateStore() -> broken into applyParentStoreProperties() and initStore() and refactored.
  • updateView() -> migrated to GraphConfig.java and renamed initView().
  • validateAndUpdateGetFromCacheHook() -> migrated to GraphConfig.java and heavily refactored. During refactoring a slight oddity was found that I couldn't get to the bottom of where graph hook seems to always have to be added to index 0 of the list of hooks else tests fail. See line 229 in GraphConfig.java.

GraphConfig.java

  • Added logging.
  • General code smells addressed and minor refactoring.

Other

  • Added custom exception classes for graph hook errors and refactored tests to look for them.
  • Use assertJ where possible.
  • Javadocs

Related issue

@tb06904 tb06904 self-assigned this Oct 23, 2023
@tb06904 tb06904 linked an issue Oct 23, 2023 that may be closed by this pull request
Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

I think there are a few bits of logic which have changed, could that be explained? It's difficult to tell because of how GitHub has formatted the diff.

@GCHQDeveloper314 GCHQDeveloper314 changed the title Gh-3018 migrate graphconfig code out of graph Gh-3018: Migrate GraphConfig code out of Graph Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (ae8c2ed) 65.36% compared to head (5b3af08) 65.39%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #3050      +/-   ##
=============================================
+ Coverage      65.36%   65.39%   +0.03%     
+ Complexity      2646     2643       -3     
=============================================
  Files            907      909       +2     
  Lines          28981    28993      +12     
  Branches        3249     3235      -14     
=============================================
+ Hits           18942    18960      +18     
+ Misses          8581     8579       -2     
+ Partials        1458     1454       -4     
Files Coverage Δ
...affer/graph/hook/exception/GraphHookException.java 100.00% <100.00%> (ø)
...graph/hook/exception/GraphHookSuffixException.java 50.00% <50.00%> (ø)
...ain/java/uk/gov/gchq/gaffer/graph/GraphConfig.java 67.70% <88.40%> (+12.43%) ⬆️
.../src/main/java/uk/gov/gchq/gaffer/graph/Graph.java 66.66% <75.00%> (-0.09%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t92549
Copy link
Contributor

t92549 commented Oct 26, 2023

@tb06904 Really nice changes and refactoring! A lot easier to follow what is happening now and the logging is improved

@tb06904 tb06904 requested a review from t92549 November 2, 2023 11:02
@tb06904 tb06904 added this to the v2.1.0 milestone Nov 2, 2023
t92549
t92549 previously approved these changes Nov 2, 2023
@t92549 t92549 removed this from the v2.1.0 milestone Nov 2, 2023
Copy link
Contributor

@GCHQDev404 GCHQDev404 left a comment

Choose a reason for hiding this comment

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

So In the past I've lightly tried to sort out Graph.Builder() but it wasn't a good choice while also adding extra functionality so I revert it out I think twice. This seems like a good refactor. The logic which I care about seems to be retained and function equivelently. I just have a couple points I'd like you to answer.

Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

Changes to these files appear to be out of scope of the original issue:

  • GraphFactoryTest.java
  • ElementUtil.java
  • ExportToGafferResultCacheHandlerTest.java
  • GetGafferResultCacheExportHandlerTest.java

@t92549 t92549 dismissed GCHQDev404’s stale review November 10, 2023 17:09

Comments addressed

@t92549 t92549 merged commit c7b2bb9 into develop Nov 10, 2023
25 checks passed
@t92549 t92549 deleted the gh-3018-migrate-graphconfig-code-out-of-graph branch November 10, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate GraphConfig code out of Graph
4 participants