ocket8888 commented on a change in pull request #6235:
URL: https://github.com/apache/trafficcontrol/pull/6235#discussion_r716939034
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -79,6 +78,35 @@ func Get(w http.ResponseWriter, r *http.Request) {
}
}
+// Get is the handler for GET requests to /logs V4.0.
+func Getv40(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, []string{"days",
"limit"})
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+ days := DefaultLogDays
+ if pDays, ok := inf.IntParams["days"]; ok {
+ days = pDays
+ }
+
+ a := tc.Alerts{}
+ setLastSeenCookie(w)
+ logs, count, err := getLogV40(inf, days)
+
+ if err != nil {
+ a.AddNewAlert(tc.ErrorLevel, err.Error())
+ api.WriteAlerts(w, r, http.StatusInternalServerError, a)
Review comment:
this should probably just use `api.HandleErr` instead of crafting alerts
in here
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -135,11 +163,64 @@ FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
const countQuery = `SELECT count(l.tm_user) FROM log as l`
+func getLogV40(inf *api.APIInfo, days int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": {Column: "u.username", Checker: nil},
+ }
+ where, _, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params,
queryParamsToQueryCols)
+
+ if len(errs) > 0 {
+ return nil, 0, util.JoinErrs(errs)
+ }
+
+ timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v'
DAY", days)
Review comment:
nit but formatting that isn't meant to layout data structures (which
should, itself, be rare) should not use `%v`, since a more specific formatting
parameter would enforce a compile-time check on the type of the related argument
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -135,11 +163,64 @@ FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
const countQuery = `SELECT count(l.tm_user) FROM log as l`
+func getLogV40(inf *api.APIInfo, days int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": {Column: "u.username", Checker: nil},
+ }
+ where, _, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params,
queryParamsToQueryCols)
+
+ if len(errs) > 0 {
+ return nil, 0, util.JoinErrs(errs)
+ }
+
+ timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v'
DAY", days)
+ if where != "" {
+ whereCount = ", tm_user as u\n" + where + " AND l.tm_user =
u.id"
+ where = where + " AND " + timeInterval
+ } else {
+ whereCount = where
+ where = "\nWHERE " + timeInterval
+ }
+ queryCount := countQuery + whereCount
+ rowCount, err := inf.Tx.NamedQuery(queryCount, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying log count for a given
user: " + err.Error())
+ }
+ defer rowCount.Close()
+ for rowCount.Next() {
+ if err = rowCount.Scan(&count); err != nil {
+ return nil, count, errors.New("scanning logs: " +
err.Error())
+ }
+ }
+
+ query := selectFromQuery + where + "\n ORDER BY last_updated DESC" +
pagination
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying logs: " + err.Error())
+ }
+ defer rows.Close()
+ ls := []tc.Log{}
+ for rows.Next() {
+ l := tc.Log{}
+ if err = rows.Scan(&l.ID, &l.Level, &l.Message, &l.User,
&l.TicketNum, &l.LastUpdated); err != nil {
+ return nil, count, errors.New("scanning logs: " +
err.Error())
Review comment:
Same as above RE: error wrapping
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -135,11 +163,64 @@ FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
const countQuery = `SELECT count(l.tm_user) FROM log as l`
+func getLogV40(inf *api.APIInfo, days int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": {Column: "u.username", Checker: nil},
+ }
+ where, _, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params,
queryParamsToQueryCols)
+
+ if len(errs) > 0 {
+ return nil, 0, util.JoinErrs(errs)
+ }
+
+ timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v'
DAY", days)
+ if where != "" {
+ whereCount = ", tm_user as u\n" + where + " AND l.tm_user =
u.id"
+ where = where + " AND " + timeInterval
+ } else {
+ whereCount = where
+ where = "\nWHERE " + timeInterval
+ }
+ queryCount := countQuery + whereCount
+ rowCount, err := inf.Tx.NamedQuery(queryCount, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying log count for a given
user: " + err.Error())
Review comment:
errors created from errors should wrap using e.g. `fmt.Errorf("querying
log count for a given user: %w", err)`
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -79,6 +78,35 @@ func Get(w http.ResponseWriter, r *http.Request) {
}
}
+// Get is the handler for GET requests to /logs V4.0.
+func Getv40(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, []string{"days",
"limit"})
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+ days := DefaultLogDays
+ if pDays, ok := inf.IntParams["days"]; ok {
+ days = pDays
+ }
+
+ a := tc.Alerts{}
+ setLastSeenCookie(w)
+ logs, count, err := getLogV40(inf, days)
+
+ if err != nil {
+ a.AddNewAlert(tc.ErrorLevel, err.Error())
+ api.WriteAlerts(w, r, http.StatusInternalServerError, a)
+ return
+ }
+ if a.HasAlerts() {
Review comment:
this will never be the case. The only code path that adds an alert
writes it and then returns, so this will never evaluate to true
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -135,11 +163,64 @@ FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
const countQuery = `SELECT count(l.tm_user) FROM log as l`
+func getLogV40(inf *api.APIInfo, days int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": {Column: "u.username", Checker: nil},
+ }
+ where, _, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params,
queryParamsToQueryCols)
+
+ if len(errs) > 0 {
+ return nil, 0, util.JoinErrs(errs)
+ }
+
+ timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v'
DAY", days)
+ if where != "" {
+ whereCount = ", tm_user as u\n" + where + " AND l.tm_user =
u.id"
+ where = where + " AND " + timeInterval
+ } else {
+ whereCount = where
+ where = "\nWHERE " + timeInterval
+ }
+ queryCount := countQuery + whereCount
+ rowCount, err := inf.Tx.NamedQuery(queryCount, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying log count for a given
user: " + err.Error())
+ }
+ defer rowCount.Close()
+ for rowCount.Next() {
+ if err = rowCount.Scan(&count); err != nil {
+ return nil, count, errors.New("scanning logs: " +
err.Error())
Review comment:
Same as above RE: error wrapping
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -135,11 +163,64 @@ FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
const countQuery = `SELECT count(l.tm_user) FROM log as l`
+func getLogV40(inf *api.APIInfo, days int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": {Column: "u.username", Checker: nil},
+ }
+ where, _, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params,
queryParamsToQueryCols)
+
+ if len(errs) > 0 {
+ return nil, 0, util.JoinErrs(errs)
+ }
+
+ timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v'
DAY", days)
+ if where != "" {
+ whereCount = ", tm_user as u\n" + where + " AND l.tm_user =
u.id"
+ where = where + " AND " + timeInterval
+ } else {
+ whereCount = where
+ where = "\nWHERE " + timeInterval
+ }
+ queryCount := countQuery + whereCount
+ rowCount, err := inf.Tx.NamedQuery(queryCount, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying log count for a given
user: " + err.Error())
+ }
+ defer rowCount.Close()
+ for rowCount.Next() {
+ if err = rowCount.Scan(&count); err != nil {
+ return nil, count, errors.New("scanning logs: " +
err.Error())
+ }
+ }
+
+ query := selectFromQuery + where + "\n ORDER BY last_updated DESC" +
pagination
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying logs: " + err.Error())
Review comment:
Same as above RE: error wrapping
##########
File path: docs/source/api/v4/logs.rst
##########
@@ -37,7 +37,7 @@ Request Structure
+===========+==========+=====================================================================================================================================+
| days | no | An integer number of days of change logs to
return
|
+-----------+----------+-------------------------------------------------------------------------------------------------------------------------------------+
- | limit | no | The number of records to which to limit the
response
|
+ | limit | no | The number of records to which to limit the
response, by default it will display all the response
|
Review comment:
This table is now malformed, needs to be aligned - but also, you don't
need to make mention of there not being a default, instead the older API
versions should make it clear that there _is_ a default. One would expect that
if a limit is not given, then there is no limit. The docs should just clarify
when those expectations are not met.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
