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

Fix #2865: Thread should have its own message and posts should only be the replies #2944

Merged
merged 3 commits into from Feb 23, 2022
Merged

Fix #2865: Thread should have its own message and posts should only be the replies #2944

merged 3 commits into from Feb 23, 2022

Conversation

vivekratnavel
Copy link
Contributor

Describe your changes :

See #2865

Type of change :

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

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

@github-actions
Copy link

Schema Change Detected. Needs ingestion-core version bump

Please run make core_bump_version_dev in the project's root and commit the changes to _version.py in this PR. Please ignore if this has been handled already.

mithmatt
mithmatt reviewed Feb 23, 2022
View reviewed changes
catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java Outdated
user.setRoles(fields.contains("roles") ? getRoles(user) : null);
user.setOwns(fields.contains("owns") ? getOwns(user) : null);
user.setFollows(fields.contains("follows") ? getFollows(user) : null);
if (fields != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that Fields is never null and we use Fields.EMPTY_FIELDS instead.

Is there a place where this assumption is not valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. We don't pass null anywhere for Fields. Removed this null check in the latest patch.

mithmatt
mithmatt reviewed Feb 23, 2022
View reviewed changes
catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java Outdated
if (uniqueValues.add(t)) {
threads.add(EntityUtil.validate(t, dao.feedDAO().findById(t), Thread.class));
private List<Thread> limitPostsInThreads(List<Thread> threads, int limitPosts) {
if (limitPosts == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If limit is 0, we return all posts?

Should we do t.withPosts(null) when limit = 0? As in return 0 posts per thread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this patch, both t.withPosts(null) and t.withPosts(0) will return all the posts in the thread. Do you think we should add support for returning just the thread with 0 posts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was possibly misled when I saw that we don't return 0 posts when limit is 0.

From a UX point of view, it seems like we'll always need a non-zero number of posts to show our users. So this logic is good.

May be add a comment to this line indicating the above, for future reference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Second thought:
May be also check what is the behavior with pagination and do something similar here.
What happens if we put limit 0 for pagination of GET /tables?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For pagination:

      @Parameter(description = "Limit the number tables returned. (1 to 1000000, default = 10)")
          @DefaultValue("10")
          @Min(1)
          @Max(1000000)
          @QueryParam("limit")

So all we have to do is document the API accordingly similar to the one above (in FeedResource.java)
Indicate that if limit = 0 it returns everything. -- so this is clear to end users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added a comment and unit test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the min and max limits in the API for limitPosts and updated the tests. Please take another look. Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks much better with API docs - thanks for incorporating this (especially as I was thinking back and forth through this and providing feedback on the fly)

mithmatt
mithmatt approved these changes Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@mithmatt mithmatt left a comment

Choose a reason for hiding this comment

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

Logic looks good.

Need to enhance the feeds threadlist API documentation so that the limit values are clear to the end user.

@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

96.9% 96.9% Coverage
0.0% 0.0% Duplication

@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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mithmatt
Copy link
Collaborator

mithmatt commented Feb 23, 2022 •
edited

Test failure from possible flaky test

2022-02-23T22:21:59.7902042Z [INFO] Results:
2022-02-23T22:21:59.7902225Z [INFO] 
2022-02-23T22:21:59.7902398Z [ERROR] Errors: 
2022-02-23T22:21:59.7905259Z [ERROR]   RoleResourceTest.defaultRole:73->createRolesAndSetDefault:124->EntityResourceTest.getEntityByName:1305 » HttpResponse
2022-02-23T22:21:59.7905632Z [INFO] 
2022-02-23T22:21:59.7905869Z [ERROR] Tests run: 891, Failures: 0, Errors: 1, Skipped: 0

Will address this separately.

@mithmatt mithmatt mentioned this pull request Feb 23, 2022
Closed
@vivekratnavel
Copy link
Contributor Author

Test failure is not related to this patch. So, merging it into main. Thanks, @mithmatt for the quick review.

@vivekratnavel vivekratnavel merged commit 906f5c5 into open-metadata:main Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mithmatt mithmatt mithmatt approved these changes

@harshach harshach Awaiting requested review from harshach

@sureshms sureshms Awaiting requested review from sureshms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vivekratnavel @mithmatt