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

Refactor EntityReference list population with id list for a given entity type #2956

Merged
merged 1 commit into from Feb 23, 2022
Merged

Refactor EntityReference list population with id list for a given entity type #2956

merged 1 commit into from Feb 23, 2022

Conversation

mithmatt
Copy link
Collaborator

Describe your changes :

I worked on this to simplify the codebase

Type of change :

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Frontend Preview (Screenshots) :

For frontend related change, please link screenshots of your changes preview! Optional for backend related changes.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

Backend: @sureshms @harshach

@mithmatt mithmatt requested review from sureshms, pmbrull, harshach and vivekratnavel and removed request for pmbrull February 23, 2022 21:28
@mithmatt mithmatt self-assigned this Feb 23, 2022
vivekratnavel
vivekratnavel approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@vivekratnavel vivekratnavel left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@vivekratnavel
Copy link
Contributor

A couple of tests are failing. We might want to check if it is related:

UserResourceTest>EntityResourceTest.get_entityIncludeDeleted_200:652->EntityResourceTest.getEntity:1279 » NullPointer
2022-02-23T21:41:22.4430100Z [ERROR]   UserResourceTest>EntityResourceTest.get_entityListWithPagination_200:479->EntityResourceTest.deleteAndCheckEntity:1325->EntityResourceTest.deleteAndCheckEntity:1349->EntityResourceTest.getEntity:1289 » NullPointer
2022-02-23T21:41:22.4431307Z [ERROR]   UserResourceTest>EntityResourceTest.get_entityWithDifferentFields_200_OK:628->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4432470Z [ERROR]   UserResourceTest.get_listUsersWithTeams_200_ok:278->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4433374Z [ERROR]   UserResourceTest>EntityResourceTest.patch_entityAttributes_200_ok:1062->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4434215Z [ERROR]   UserResourceTest.patch_userAttributes_as_admin_200_ok:443->EntityResourceTest.patchEntityAndCheck:1505->EntityResourceTest.patchEntity:1321 » NullPointer
2022-02-23T21:41:22.4435157Z [ERROR]   UserResourceTest.post_userWithDefaultRole:113->createUserAndCheckRoles:556->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4436131Z [ERROR]   UserResourceTest.post_validAdminUser_200_ok:217->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4437112Z [ERROR]   UserResourceTest.post_validUserWithRoles_200_ok:257->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4438108Z [ERROR]   UserResourceTest.post_validUser_200_ok:158->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4439098Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityCreate_200:776->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4440092Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityEmptyDescriptionUpdate_200:900->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4441233Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityNonEmptyDescriptionUpdate_200:907->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4442330Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityNullDescriptionUpdate_200:884->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4443454Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityUpdateWithNoChange_200:783->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4482927Z [ERROR]   UserResourceTest.put_validUser_200_ok:177->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4484349Z [ERROR]   TopicResourceTest>EntityResourceTest.put_entityUpdateOwner_200:836->EntityResourceTest.checkOwnerOwns:1797->EntityResourceTest.getEntity:1279 » NullPointer

@mithmatt
Copy link
Collaborator Author

A couple of tests are failing. We might want to check if it is related:

UserResourceTest>EntityResourceTest.get_entityIncludeDeleted_200:652->EntityResourceTest.getEntity:1279 » NullPointer
2022-02-23T21:41:22.4430100Z [ERROR]   UserResourceTest>EntityResourceTest.get_entityListWithPagination_200:479->EntityResourceTest.deleteAndCheckEntity:1325->EntityResourceTest.deleteAndCheckEntity:1349->EntityResourceTest.getEntity:1289 » NullPointer
2022-02-23T21:41:22.4431307Z [ERROR]   UserResourceTest>EntityResourceTest.get_entityWithDifferentFields_200_OK:628->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4432470Z [ERROR]   UserResourceTest.get_listUsersWithTeams_200_ok:278->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4433374Z [ERROR]   UserResourceTest>EntityResourceTest.patch_entityAttributes_200_ok:1062->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4434215Z [ERROR]   UserResourceTest.patch_userAttributes_as_admin_200_ok:443->EntityResourceTest.patchEntityAndCheck:1505->EntityResourceTest.patchEntity:1321 » NullPointer
2022-02-23T21:41:22.4435157Z [ERROR]   UserResourceTest.post_userWithDefaultRole:113->createUserAndCheckRoles:556->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4436131Z [ERROR]   UserResourceTest.post_validAdminUser_200_ok:217->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4437112Z [ERROR]   UserResourceTest.post_validUserWithRoles_200_ok:257->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4438108Z [ERROR]   UserResourceTest.post_validUser_200_ok:158->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4439098Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityCreate_200:776->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4440092Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityEmptyDescriptionUpdate_200:900->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4441233Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityNonEmptyDescriptionUpdate_200:907->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4442330Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityNullDescriptionUpdate_200:884->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4443454Z [ERROR]   UserResourceTest>EntityResourceTest.put_entityUpdateWithNoChange_200:783->EntityResourceTest.createAndCheckEntity:1375->EntityResourceTest.createAndCheckEntity:1397->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4482927Z [ERROR]   UserResourceTest.put_validUser_200_ok:177->EntityResourceTest.updateAndCheckEntity:1423->EntityResourceTest.getEntity:1273 » NullPointer
2022-02-23T21:41:22.4484349Z [ERROR]   TopicResourceTest>EntityResourceTest.put_entityUpdateOwner_200:836->EntityResourceTest.checkOwnerOwns:1797->EntityResourceTest.getEntity:1279 » NullPointer

Yes, this is very likely from this PR.

Looking into resolving them.

In brief: we can either return null or empty list - we tend to use a mix across the codebase.

@sonarcloud
Copy link

sonarcloud bot commented Feb 23, 2022

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Feb 23, 2022

[catalog] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

78.6% 78.6% Coverage
0.0% 0.0% Duplication

@mithmatt
Copy link
Collaborator Author

mithmatt commented Feb 23, 2022 •
edited

Fixed the NPE bug (was a miss on my side).
All tests passed
Merging...

@mithmatt mithmatt merged commit c1a1447 into open-metadata:main Feb 23, 2022
@mithmatt mithmatt deleted the simplify_er_population branch February 23, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vivekratnavel vivekratnavel vivekratnavel approved these changes

@sureshms sureshms Awaiting requested review from sureshms

@harshach harshach Awaiting requested review from harshach

Assignees

@mithmatt mithmatt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mithmatt @vivekratnavel