Traffic Ops Rewrite: /api/1.2/servers endpoint - with sqlx #1146
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
-1 on including The cost far outweighs the benefit. |
|
Refer to this link for build results (access rights to CI server needed): |
dewrich
changed the title
dewrich
added
the
WIP
dewrich
changed the title
| "net/url" | ||
|
|
||
| "github.com/apache/incubator-trafficcontrol/traffic_monitor_golang/common/log" | ||
| . "github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/tcstructs" |
There was a problem hiding this comment.
We shouldn't use unqualified imports. I know it's less typing, but Unqualified imports poison the namespace, and make it hard to figure out where things are coming from.
I'd support a shorter package name, e.g. tc for tc.Server.
| cdns := []Cdn{} | ||
|
|
||
| if err != nil { | ||
| panic(err.Error()) // proper error handling instead of panic in your app |
There was a problem hiding this comment.
Need to return the error, not panic and crash the app.
There was a problem hiding this comment.
Ah, this whole function is never executed, because the err is previously checked and returned. Needs removed.
There was a problem hiding this comment.
must have been a cut/paste issue
|
|
||
| const HiddenField = "********" | ||
| if err != nil { | ||
| panic(err.Error()) // proper error handling instead of panic in your app |
There was a problem hiding this comment.
Err check is never hit, check and panic need removed.
| COALESCE(s.ilo_ip_netmask, '') as ilo_ip_netmask, | ||
| COALESCE(s.ilo_password, '') as ilo_password, | ||
| COALESCE(s.ilo_username, '') as ilo_username, | ||
| COALESCE(s.interface_mtu, 9000) as interface_mtu, |
There was a problem hiding this comment.
Can you put 9000 in a const somewhere?
| @@ -23,14 +23,15 @@ import ( | |||
| "database/sql" | |||
|
|
|||
| "github.com/apache/incubator-trafficcontrol/traffic_monitor_golang/common/log" | |||
| "github.com/jmoiron/sqlx" | |||
There was a problem hiding this comment.
Sqlx should be removed; cost of a large, pervasive, slow dependency outweighs shorter Scan lines.
There was a problem hiding this comment.
Developer's are error prone, and the cost of introducing this dependency is much less costly than the cost of consistent bug introduction
There was a problem hiding this comment.
Scan lines for large structs, 42 fields, could easily be improperly ordered, either when adding a new field or just pulling the object at some point.
Using structs, even anonymous ones declared in the function when you don't need every field, with annotations mean you just need to verify the fields' db annotations are named properly.
There was a problem hiding this comment.
I agree it's better. It's just not worth the cost. Not convinced annotations are easier to get right than Scan, though.
Again, when-not-if the library stops being maintained, it'll require fixing every single endpoint. More likely, like Goose, we just won't do it, and we'll live with unmaintained and vulnerable code for years.
The performance is also an issue. It's not a huge slowdown, but it does use slow Reflection. If we keep making small choices against performance, we'll end up where we are with Perl via Death By A Thousand Cuts.
There was a problem hiding this comment.
the cost of consistent bug introduction
easily be improperly ordered
If your fields are improperly ordered, if they're different types, it will immediately fail when run. Even if the types are the same, the values will be wrong, and immediately obvious when testing your code. But even if someone is pushing code without running it, we're requiring tests for every endpoint, right? Those tests should fail.
In short, I don't buy the "more bugs" argument, it would take a long cascade chain of failures, and complete lack of testing or tests, and pushing code without ever running it. You could make the same argument of any code.
There was a problem hiding this comment.
Do we know how much of a performance hit we're discussing? Can we toss together some quick benchmarks? Performance is pretty important here, but if it's a non-issue it'd be nice to know.
There was a problem hiding this comment.
@alficles Agree. Tangentially, we need to performance test Prepared queries too. Though the maintenance cost is the bigger issue.
| @@ -36,6 +36,10 @@ func Routes(d ServerData) ([]Route, http.Handler, error) { | |||
| return []Route{ | |||
| {1.2, http.MethodGet, "cdns/{cdn}/configs/monitoring", wrapHeaders(wrapAuth(monitoringHandler(d.DB), d.Insecure, d.TOSecret, rd.PrivLevelStmt, MonitoringPrivLevel))}, | |||
| {1.2, http.MethodGet, "cdns/{cdn}/configs/monitoring.json", wrapHeaders(wrapAuth(monitoringHandler(d.DB), d.Insecure, d.TOSecret, rd.PrivLevelStmt, MonitoringPrivLevel))}, | |||
| {1.2, http.MethodGet, "servers", wrapHeaders(wrapAuthWithData(serversHandler(d.DB), d.Insecure, d.TOSecret, rd.PrivLevelStmt, ServersPrivLevel))}, | |||
| {1.2, http.MethodGet, "servers.json", wrapHeaders(wrapAuthWithData(serversHandler(d.DB), d.Insecure, d.TOSecret, rd.PrivLevelStmt, ServersPrivLevel))}, | |||
| {1.2, http.MethodGet, "cdns", wrapHeaders(wrapAuthWithData(cdnsHandler(d.DB), d.Insecure, d.TOSecret, rd.PrivLevelStmt, ServersPrivLevel))}, | |||
There was a problem hiding this comment.
The cdns endpoint should have its own CdnsPrivLevel
| return s[i].Name < s[j].Name | ||
| } | ||
|
|
||
| func sortCdns(p []Cdn) []Cdn { |
There was a problem hiding this comment.
I'd vote not using this func, and directly calling sort.Sort(SortableCdns(p)), because it's deceptive, it looks like it's a pure function that takes and returns without mutating, but it actually modifies the input. And it's a single line.
| for rows.Next() { | ||
| var s Cdn | ||
| err = rows.StructScan(&s) | ||
| if err != nil { |
There was a problem hiding this comment.
Nitpick: this can be collapsed to if err := rows.StructScan(&s); err != nil {
| if err != nil { | ||
| //TODO: drichardson - send back an alert if the Query Count is larger than 1 | ||
| // Test for bad Query Parameters | ||
| return nil, err |
There was a problem hiding this comment.
Error context would help debugging
There was a problem hiding this comment.
We need to support the existing TO API which supports the "alerts" json response in the event of an error to report to the API Client
|
|
||
| const HiddenField = "********" | ||
| if err != nil { | ||
| panic(err.Error()) // proper error handling instead of panic in your app |
There was a problem hiding this comment.
Check is unused, check and panic need removed
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
dewrich
changed the title
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
(cherry picked from commit 29cc755)
|
Refer to this link for build results (access rights to CI server needed): |
dewrich
force-pushed
the
tor-api-servers
branch
from
76c29d3
to
721b952
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
No description provided.