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

refactor(gms): Refactoring util + entity client class locations #5902

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

jjoyce0510
Copy link
Collaborator

Summary

Minor refactoring PR to move location of JavaEntityClient and various util methods / classes into more suitable locations.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Sep 12, 2022

Unit Test Results (build & test)

571 tests  ±0   571 ✔️ ±0   14m 18s ⏱️ -10s
141 suites ±0       0 💤 ±0 
141 files   ±0       0 ±0 

Results for commit 5003840. ± Comparison against base commit 386719f.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

I have one question but not blocking an approval!

Comment on lines +690 to +708
// Creates new Filter from a map of Criteria by removing null-valued Criteria and using EQUAL condition (default).
@Nonnull
public static Filter newFilter(@Nullable Map<String, String> params) {
if (params == null) {
return new Filter().setOr(new ConjunctiveCriterionArray());
}
CriterionArray criteria = params.entrySet()
.stream()
.filter(e -> Objects.nonNull(e.getValue()))
.map(e -> newCriterion(e.getKey(), e.getValue(), Condition.EQUAL))
.collect(Collectors.toCollection(CriterionArray::new));
return new Filter().setOr(
new ConjunctiveCriterionArray(ImmutableList.of(new ConjunctiveCriterion().setAnd(criteria))));
}

@Nonnull
public static Criterion newCriterion(@Nonnull String field, @Nonnull String value, @Nonnull Condition condition) {
return new Criterion().setField(field).setValue(value).setCondition(condition);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curios - why duplicate these from QueryUtils to here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Query Utils are unfortunately inside of metadata-io which should be a library that is NOT exposed to folks who just want to use a RestliEntityClient... It's too heavy. So we've removed this dependency completely and as part of that we had to duplicate this shared logic. That being said, I think it'd be better moving forward to establish a module where truly "shared" libraries can reside. I'm going to add a TODO here

@jjoyce0510 jjoyce0510 merged commit 450fb2e into datahub-project:master Sep 12, 2022
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.

2 participants