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

Traffic Ops GET /api/*/cdns should be sorted alphabetically by name by default #3455

Closed
rawlinp opened this issue Apr 1, 2019 · 6 comments · Fixed by #5064
Closed

Traffic Ops GET /api/*/cdns should be sorted alphabetically by name by default #3455

rawlinp opened this issue Apr 1, 2019 · 6 comments · Fixed by #5064
Labels
good first issue first-time committers will find this easy to resolve hacktoberfest low impact affects only a small portion of a CDN, and cannot itself break one regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops

Comments

@rawlinp
Copy link
Contributor

rawlinp commented Apr 1, 2019

When clicking the snapshot button (the camera icon in the top right) in Traffic Portal, it shows the list of CDNs to choose from. Today, these CDNs appear to be sorted ascending by their numerical ID rather than alphabetically by their name. With a high number of CDNs to choose from, it would be easier to sift through the list alphabetically.

The CDN table (viewed by clicking the "CDNs" tab on the left) should also be sorted alphabetically by name. This appears to be a minor regression from the Perl TO API implementation which sorted by name by default (if no other orderby param is specified in the request URL): https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/API/Cdn.pm#L34

@rawlinp rawlinp added new feature A new feature, capability or behavior Traffic Portal v1 related to Traffic Portal version 1 labels Apr 1, 2019
@rawlinp rawlinp changed the title List of CDNs to snapshot should be sorted alphabetically by name List of CDNs should be sorted alphabetically by name Apr 1, 2019
@rawlinp rawlinp added low impact affects only a small portion of a CDN, and cannot itself break one regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops and removed new feature A new feature, capability or behavior Traffic Portal v1 related to Traffic Portal version 1 labels Apr 1, 2019
@rawlinp rawlinp changed the title List of CDNs should be sorted alphabetically by name Traffic Ops GET /api/*/cdns should be sorted alphabetically by name by default Apr 1, 2019
@rawlinp rawlinp changed the title Traffic Ops GET /api/*/cdns should be sorted alphabetically by name by default Traffic Ops GET /api/*/cdns should be sorted alphabetically by name by default Apr 1, 2019
@dangogh
Copy link
Member

dangogh commented Apr 2, 2019

actually, I used to agree, but I've changed my mind and disagree.. a client should not be dependent on the order items are returned. If it has the need for them to be ordered a specific way, it should do so itself. In the case of the portal, it's easy enough to sort them when the cdn list is retrieved. We do have precedent for other types (I think servers are sorted), but we shouldn't try to do that going forward..

@dangogh
Copy link
Member

dangogh commented Apr 2, 2019

in other words, I think this should be against the portal and not traffic ops

@rawlinp
Copy link
Contributor Author

rawlinp commented Apr 2, 2019

Yeah, I'm just thinking it's a minor change in behavior from the Perl implementation. Really, I think TP should be sending the ?orderby=name qparam in its request to TO to explicitly request that ordering. If you think this minor change in behavior is ok then I can get on board with that, and we can just fix the API request in TP.

@rob05c
Copy link
Member

rob05c commented Apr 2, 2019

+1 on putting it in the Portal, not the API

IMO sorting shouldn't be part of an API. It's trivial for clients to do, and it reduces the scalability of the server. Sometimes it's trivial for the server to do, sometimes it's expensive. If it's expensive, it means that for n clients, the server has to do O(n) work, where the work for the client is O(1). And the client code is trivial as well, vals = http.Get(url); vals = sort(vals). IMO there's simply no good reason for clients to need the server to sort, and there's a potentially very good reason for the server not to.

@rawlinp
Copy link
Contributor Author

rawlinp commented Apr 2, 2019

I think we are getting into a much broader discussion about API 2.0. Currently most of our 1.x endpoints support server-side ordering, and I don't think we should rip that out of the 1.x endpoints. If we feel strongly that API 2.0 shouldn't support server-side ordering, then that's a discussion we should have on the mailing list about API 2.0 at a good time.

For this specific issue, I don't really care strongly if the sorting is done client-side or server-side, but I also don't think we should be worried about server-side sorting of CDNs affecting the scalability of TO. I would merge a TP PR that did the sorting either way.

@ocket8888
Copy link
Contributor

ocket8888 commented Apr 11, 2019

I think the API should impose some order on the results it returns. Whether or not it's configurable via e.g. an orderBy query parameter is something I don't feel strongly about, but when things cannot be guaranteed to have any specific order it means that two requests to the same endpoint with the same TODB dataset cannot be guaranteed to have the same response, which harms testability. Imposing a specific ordering would eliminate a full third of the use case of my PR #3478

@mitchell852 mitchell852 added the bug something isn't working as intended label Oct 2, 2019
@mitchell852 mitchell852 added good first issue first-time committers will find this easy to resolve hacktoberfest and removed bug something isn't working as intended labels Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue first-time committers will find this easy to resolve hacktoberfest low impact affects only a small portion of a CDN, and cannot itself break one regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants