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

ARTEMIS-2285 Sorting by user column in connections view not working #2592

Closed
wants to merge 1 commit into from
Closed

Conversation

sebthom
Copy link
Contributor

@sebthom sebthom commented Mar 26, 2019

The value returned by ConnectionsView.getField(con, "users") currently returns a Set which does not implement the Comparable interface thus ActiveMQAbstractView#getComparator() always returns a comparator which evaluates to 0.

@gaohoward
Copy link
Contributor

looking good. Could you add a unit test?

for (ServerSession session : sessions) {
String username = session.getUsername() == null ? "" : session.getUsername();
users.add(username);
}
return users;
return StringUtil.joinStringList(users, ",");
Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst i understand change from HashSet to TreeSet to give ordering, the return shouldn't change the type, e.g just return users still. Or im missing something.... i guess a test may help to describe/show why its needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The returned value is a set of users of a single connection. to sort the connections by their user(s), these lists need to be comparable with each other. however treeset does not implement the comparable interface, so you cannot sort multiple treesets by there values. thus the PR now returns a comparable/sortable string representation of the set. does that make sense?

@sebthom
Copy link
Contributor Author

sebthom commented Apr 10, 2019

I will try to provide a test case. stay tuned :)

@sebthom
Copy link
Contributor Author

sebthom commented Apr 15, 2019

@gaohoward @michaelandrepearce I added a test as requested.

@asfgit asfgit closed this in 65796c9 Apr 15, 2019
@michaelandrepearce
Copy link
Contributor

@sebthom thanks for this great contributions. merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants