You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "gbkannan89 (via GitHub)" <gi...@apache.org> on 2023/08/04 06:47:54 UTC
[GitHub] [trafficcontrol] gbkannan89 opened a new pull request, #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
gbkannan89 opened a new pull request, #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698
Related: #5911
<hr/>
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops
## What is the best way to verify this PR?
Curl all Region v5 REST calls (GET, CREATE, UPDATE, DELETE) and ensure you are getting RFC3339 date-time ( eg: lastUpdated : "2023-05-25T15:59:33.7096-06:00" )
## If this is a bugfix, which Traffic Control versions contained the bug?
- 7.0.x
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] kdamichie commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "kdamichie (via GitHub)" <gi...@apache.org>.
kdamichie commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1284732002
##########
docs/source/api/v5/regions.rst:
##########
@@ -126,19 +124,16 @@ Request Structure
Content-Type: application/json
{
- "name": "Manchester",
- "division": "4",
- "divisionName": "England"
+ "division": 4,
+ "name": "Manchester"
}
-.. [1] The only "division" key that actually matters in the request body is ``division``; ``divisionName`` is not validated and has no effect - particularly not the effect of re-naming the division - beyond changing the name in the API response to this request. Subsequent requests will reveal the true name of the division. Note that if ``divisionName`` is not present in the request body it will be ``null`` in the response, but again further requests will show the true division name (provided it has been assigned to a division).
-
Response Structure
------------------
:divisionName: The name of the division which contains this region
Review Comment:
Shouldn't `divisionName` be removed in the documentation here as well?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman merged pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman merged PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1286134757
##########
traffic_ops/testing/api/v5/regions_test.go:
##########
@@ -298,7 +294,7 @@ func validateRegionsDescSort() utils.CkReqFunc {
func validateRegionsPagination(paginationParam string) utils.CkReqFunc {
return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, _ tc.Alerts, _ error) {
- paginationResp := resp.([]tc.Region)
+ paginationResp := resp.([]tc.RegionV5)
Review Comment:
This line panics for me. You're typecasting to `[]tc.RegionV5`, which is not the type of the `Response` field:
https://github.com/apache/trafficcontrol/blob/0d79728bbd9d12e8464a294bb33409ca2282c68c/lib/go-tc/regions.go#L44-L45
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1288645687
##########
lib/go-tc/regions.go:
##########
@@ -1,5 +1,7 @@
package tc
+import "time"
+
Review Comment:
Imports should be after the Apache license, not before.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] kdamichie commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "kdamichie (via GitHub)" <gi...@apache.org>.
kdamichie commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1286076862
##########
docs/source/api/v5/regions.rst:
##########
@@ -41,8 +41,7 @@ Request Structure
+-----------+----------+---------------------------------------------------------------------------------------------------------------+
| name | no | Filter :term:`Regions` by name |
+-----------+----------+---------------------------------------------------------------------------------------------------------------+
- | orderby | no | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` |
- | | | array |
+ | orderby | no | Choose the ordering of the results - either one of them "division", "id", "name" |
Review Comment:
Missing ending pipe
##########
docs/source/api/v5/regions_id.rst:
##########
@@ -39,7 +39,6 @@ Request Structure
+------+---------------------------------------------------------+
:division: The new integral, unique identifier of the division which shall contain the region\ [1]_
Review Comment:
Target "1" was removed, so his does not refer to anything anymore. Needs removed
##########
docs/source/api/v5/regions.rst:
##########
@@ -111,7 +110,6 @@ Creates a new region
Request Structure
-----------------
:division: The integral, unique identifier of the division which shall contain the new region\ [1]_
Review Comment:
Target "1" was removed, so his does not refer to anything anymore. Needs removed
##########
traffic_ops/traffic_ops_golang/region/regions.go:
##########
@@ -192,3 +200,230 @@ func deleteQuery() string {
query := deleteQueryBase() + ` WHERE id=:id`
return query
}
+
+// Read is the handler for GET requests to Regions of APIv5
+func Read(w http.ResponseWriter, r *http.Request) {
+ var maxTime time.Time
+ var runSecond bool
+
+ inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx
+ if sysErr != nil || userErr != nil {
+ api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ // Query Parameters to Database Query column mappings
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "division": {Column: "r.division"},
+ "id": {Column: "r.id", Checker: api.IsInt},
+ "name": {Column: "r.name"},
+ }
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "name"
+ }
+ where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
+ if len(errs) > 0 {
+ api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, util.JoinErrs(errs), nil)
+ }
+ if inf.Config.UseIMS {
+ runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, r.Header, queryValues, SelectMaxLastUpdatedQuery(where))
+ if !runSecond {
+ log.Debugln("IMS HIT")
+ api.AddLastModifiedHdr(w, maxTime)
+ w.WriteHeader(http.StatusNotModified)
+ return
+ }
+ log.Debugln("IMS MISS")
+ } else {
+ log.Debugln("Non IMS request")
+ }
+
+ query := `SELECT r.division, d.name as divisionname, r.id, r.last_updated, r.name FROM region r
+ JOIN division d ON r.division = d.id` + where + orderBy + pagination
+ rows, err := tx.NamedQuery(query, queryValues)
+ if err != nil {
+ api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("region get: error getting region(s): %w", err))
+ }
+ defer log.Close(rows, "unable to close DB connection")
+
+ typ := tc.RegionV5{}
+ regionList := []tc.RegionV5{}
+ for rows.Next() {
+ if err = rows.Scan(&typ.Division, &typ.DivisionName, &typ.ID, &typ.LastUpdated, &typ.Name); err != nil {
+ api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("error getting region(s): %w", err))
+ }
+ regionList = append(regionList, typ)
+ }
+
+ api.WriteResp(w, r, regionList)
+ return
+}
+
+// Create region with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+ tx := inf.Tx.Tx
+ rg, readValErr := readAndValidateJsonStruct(r)
+ if readValErr != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil)
+ return
+ }
+
+ var exists bool
+ existErr := tx.QueryRow(`SELECT EXISTS(SELECT * from region where name = $1)`, rg.Name).Scan(&exists)
+
+ if existErr != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if region with name %s exists", existErr, rg.Name))
+ return
+ }
+ if exists {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("region name '%s' already exists", rg.Name), nil)
+ return
+ }
+
+ query := `INSERT INTO region ( division, name) VALUES ( $1, $2 )
+ RETURNING
+ id,last_updated,
+ (SELECT d.name FROM division d WHERE d.id = region.division)`
+ err := tx.QueryRow(query, rg.Division, rg.Name).Scan(&rg.ID, &rg.LastUpdated, &rg.DivisionName)
+ if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error: %w in creating region with name: %s", err, rg.Name), nil)
+ return
+ }
+ usrErr, sysErr, code := api.ParseDBError(err)
+ api.HandleErr(w, r, tx, code, usrErr, sysErr)
+ return
+ }
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region is created.")
+ api.WriteAlertsObj(w, r, http.StatusCreated, alerts, rg)
+ return
+}
+
+// Update a Region for APIv5
+func Update(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ tx := inf.Tx.Tx
+ rg, readValErr := readAndValidateJsonStruct(r)
+ if readValErr != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil)
+ return
+ }
+
+ requestedRegionId := inf.IntParams["id"]
+ // check if the entity was already updated
+ userErr, sysErr, errCode = api.CheckIfUnModified(r.Header, inf.Tx, requestedRegionId, "region")
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+
+ query := `UPDATE region SET division= $1, name= $2 WHERE id= $3
+ RETURNING
+ id,last_updated,
+ (SELECT d.name FROM division d WHERE d.id = region.division)`
+
+ err := tx.QueryRow(query, rg.Division, rg.Name, requestedRegionId).Scan(&rg.ID, &rg.LastUpdated, &rg.DivisionName)
+ if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("region: %d not found", requestedRegionId), nil)
+ return
+ }
+ usrErr, sysErr, code := api.ParseDBError(err)
+ api.HandleErr(w, r, tx, code, usrErr, sysErr)
+ return
+ }
+
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region was updated")
+ api.WriteAlertsObj(w, r, http.StatusOK, alerts, rg)
+ return
+}
+
+// Delete an Region for APIv5
+func Delete(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ requestedRegionId := inf.Params["id"]
+ if requestedRegionId == "" {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("no region exists for empty id"), nil)
+ return
+ }
+
+ var exists bool
+ existErr := tx.QueryRow(`SELECT EXISTS(SELECT * from region where id = $1)`, requestedRegionId).Scan(&exists)
+
+ if existErr != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if region with id %s exists", existErr, requestedRegionId))
+ return
+ }
+ if !exists {
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("no region exists by id: %s", requestedRegionId), nil)
+ return
+ }
+
+ res, err := tx.Exec("DELETE FROM region WHERE id=$1", requestedRegionId)
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
+ return
+ }
+ rowsAffected, err := res.RowsAffected()
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("determining rows affected for delete region: %w", err))
+ return
+ }
+ if rowsAffected == 0 {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("no rows deleted for region"), nil)
+ return
+ }
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region was deleted.")
+ api.WriteAlerts(w, r, http.StatusOK, alerts)
+ return
+}
+
+// readAndValidateJsonStruct reads json body and validates json fields
+func readAndValidateJsonStruct(r *http.Request) (tc.RegionV5, error) {
+ var region tc.RegionV5
+ if err := json.NewDecoder(r.Body).Decode(®ion); err != nil {
+ userErr := fmt.Errorf("error decoding POST request body into RegionV5 struct %w", err)
+ return region, userErr
+ }
+
+ // validate JSON body
+ errs := tovalidate.ToErrors(validation.Errors{
+ "division": validation.Validate(region.Division, validation.NotNil, validation.Min(0)),
+ "name": validation.Validate(region.Name, validation.Required),
+ })
+ if len(errs) > 0 {
+ userErr := util.JoinErrs(errs)
+ return region, userErr
+ }
+ return region, nil
+}
+
+// SelectMaxLastUpdatedQuery used for TryIfModifiedSinceQuery()
+func SelectMaxLastUpdatedQuery(where string) string {
+ return `SELECT max(t) from (
+ SELECT max(r.last_updated) as t FROM region r
+JOIN division d ON r.division = d.id ` + where +
+ ` UNION ALL
+ select max(last_updated) as t from last_deleted l where l.table_name='region') as res`
Review Comment:
Any reason to keep this commented out statement?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] kdamichie commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "kdamichie (via GitHub)" <gi...@apache.org>.
kdamichie commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1284732002
##########
docs/source/api/v5/regions.rst:
##########
@@ -126,19 +124,16 @@ Request Structure
Content-Type: application/json
{
- "name": "Manchester",
- "division": "4",
- "divisionName": "England"
+ "division": 4,
+ "name": "Manchester"
}
-.. [1] The only "division" key that actually matters in the request body is ``division``; ``divisionName`` is not validated and has no effect - particularly not the effect of re-naming the division - beyond changing the name in the API response to this request. Subsequent requests will reveal the true name of the division. Note that if ``divisionName`` is not present in the request body it will be ``null`` in the response, but again further requests will show the true division name (provided it has been assigned to a division).
-
Response Structure
------------------
:divisionName: The name of the division which contains this region
Review Comment:
Shouldn't `divisionName` be removed in the documentation here as well?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] kdamichie commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "kdamichie (via GitHub)" <gi...@apache.org>.
kdamichie commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1288739968
##########
docs/source/api/v5/regions.rst:
##########
@@ -41,8 +41,7 @@ Request Structure
+-----------+----------+---------------------------------------------------------------------------------------------------------------+
| name | no | Filter :term:`Regions` by name |
+-----------+----------+---------------------------------------------------------------------------------------------------------------+
- | orderby | no | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` |
- | | | array |
+ | orderby | no | Choose the ordering of the results - either one of them "division", "id", "name" | |
Review Comment:
Stray pipe way out on this line that needs removed to make table work.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] codecov[bot] commented on pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#issuecomment-1665862126
## [Codecov](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7698](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (88faccc) into [master](https://app.codecov.io/gh/apache/trafficcontrol/commit/75ec56fa1bc4cf9c9e9ba54bde92197d4381649d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (75ec56f) will **decrease** coverage by `33.29%`.
> Report is 46 commits behind head on master.
> The diff coverage is `20.23%`.
```diff
@@ Coverage Diff @@
## master #7698 +/- ##
=============================================
- Coverage 65.05% 31.77% -33.29%
Complexity 98 98
=============================================
Files 314 497 +183
Lines 12365 39588 +27223
Branches 907 90 -817
=============================================
+ Hits 8044 12578 +4534
- Misses 3968 25842 +21874
- Partials 353 1168 +815
```
| Flag | Coverage Δ | |
|---|---|---|
| golib_unit | `48.10% <0.00%> (?)` | |
| grove_unit | `4.60% <ø> (?)` | |
| t3c_unit | `4.95% <5.88%> (?)` | |
| traffic_monitor_unit | `21.30% <0.00%> (?)` | |
| traffic_portal_v2 | `?` | |
| traffic_stats_unit | `10.14% <ø> (?)` | |
| unit_tests | `25.33% <3.67%> (-48.45%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [cache-config/t3c-apply/config/config.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2FjaGUtY29uZmlnL3QzYy1hcHBseS9jb25maWcvY29uZmlnLmdv) | `0.00% <0.00%> (ø)` | |
| [cache-config/t3c-apply/t3c-apply.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2FjaGUtY29uZmlnL3QzYy1hcHBseS90M2MtYXBwbHkuZ28=) | `0.00% <0.00%> (ø)` | |
| [lib/go-atscfg/parentdotconfig.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGliL2dvLWF0c2NmZy9wYXJlbnRkb3Rjb25maWcuZ28=) | `65.76% <ø> (ø)` | |
| [lib/go-tc/cdns.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGliL2dvLXRjL2NkbnMuZ28=) | `0.00% <ø> (ø)` | |
| [lib/go-tc/deliveryservice\_servers.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGliL2dvLXRjL2RlbGl2ZXJ5c2VydmljZV9zZXJ2ZXJzLmdv) | `0.00% <0.00%> (ø)` | |
| [lib/go-tc/federation\_resolver.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGliL2dvLXRjL2ZlZGVyYXRpb25fcmVzb2x2ZXIuZ28=) | `45.45% <0.00%> (ø)` | |
| [traffic\_monitor/tmclient/tmclient.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19tb25pdG9yL3RtY2xpZW50L3RtY2xpZW50Lmdv) | `0.00% <0.00%> (ø)` | |
| [cache-config/t3c-apply/torequest/torequest.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2FjaGUtY29uZmlnL3QzYy1hcHBseS90b3JlcXVlc3QvdG9yZXF1ZXN0Lmdv) | `5.83% <14.28%> (ø)` | |
| [traffic\_ops/v5-client/session.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L3Nlc3Npb24uZ28=) | `72.22% <50.00%> (-2.78%)` | :arrow_down: |
| [traffic\_ops/v5-client/cachegroup.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2NhY2hlZ3JvdXAuZ28=) | `63.93% <100.00%> (ø)` | |
| ... and [6 more](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [393 files with indirect coverage changes](https://app.codecov.io/gh/apache/trafficcontrol/pull/7698/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] gbkannan89 commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "gbkannan89 (via GitHub)" <gi...@apache.org>.
gbkannan89 commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1286935418
##########
traffic_ops/traffic_ops_golang/region/regions.go:
##########
@@ -192,3 +200,230 @@ func deleteQuery() string {
query := deleteQueryBase() + ` WHERE id=:id`
return query
}
+
+// Read is the handler for GET requests to Regions of APIv5
+func Read(w http.ResponseWriter, r *http.Request) {
+ var maxTime time.Time
+ var runSecond bool
+
+ inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx
+ if sysErr != nil || userErr != nil {
+ api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ // Query Parameters to Database Query column mappings
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "division": {Column: "r.division"},
+ "id": {Column: "r.id", Checker: api.IsInt},
+ "name": {Column: "r.name"},
+ }
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "name"
+ }
+ where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
+ if len(errs) > 0 {
+ api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, util.JoinErrs(errs), nil)
+ }
+ if inf.Config.UseIMS {
+ runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, r.Header, queryValues, SelectMaxLastUpdatedQuery(where))
+ if !runSecond {
+ log.Debugln("IMS HIT")
+ api.AddLastModifiedHdr(w, maxTime)
+ w.WriteHeader(http.StatusNotModified)
+ return
+ }
+ log.Debugln("IMS MISS")
+ } else {
+ log.Debugln("Non IMS request")
+ }
+
+ query := `SELECT r.division, d.name as divisionname, r.id, r.last_updated, r.name FROM region r
+ JOIN division d ON r.division = d.id` + where + orderBy + pagination
+ rows, err := tx.NamedQuery(query, queryValues)
+ if err != nil {
+ api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("region get: error getting region(s): %w", err))
+ }
+ defer log.Close(rows, "unable to close DB connection")
+
+ typ := tc.RegionV5{}
+ regionList := []tc.RegionV5{}
+ for rows.Next() {
+ if err = rows.Scan(&typ.Division, &typ.DivisionName, &typ.ID, &typ.LastUpdated, &typ.Name); err != nil {
+ api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("error getting region(s): %w", err))
+ }
+ regionList = append(regionList, typ)
+ }
+
+ api.WriteResp(w, r, regionList)
+ return
+}
+
+// Create region with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+ tx := inf.Tx.Tx
+ rg, readValErr := readAndValidateJsonStruct(r)
+ if readValErr != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil)
+ return
+ }
+
+ var exists bool
+ existErr := tx.QueryRow(`SELECT EXISTS(SELECT * from region where name = $1)`, rg.Name).Scan(&exists)
+
+ if existErr != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if region with name %s exists", existErr, rg.Name))
+ return
+ }
+ if exists {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("region name '%s' already exists", rg.Name), nil)
+ return
+ }
+
+ query := `INSERT INTO region ( division, name) VALUES ( $1, $2 )
+ RETURNING
+ id,last_updated,
+ (SELECT d.name FROM division d WHERE d.id = region.division)`
+ err := tx.QueryRow(query, rg.Division, rg.Name).Scan(&rg.ID, &rg.LastUpdated, &rg.DivisionName)
+ if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error: %w in creating region with name: %s", err, rg.Name), nil)
+ return
+ }
+ usrErr, sysErr, code := api.ParseDBError(err)
+ api.HandleErr(w, r, tx, code, usrErr, sysErr)
+ return
+ }
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region is created.")
+ api.WriteAlertsObj(w, r, http.StatusCreated, alerts, rg)
+ return
+}
+
+// Update a Region for APIv5
+func Update(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ tx := inf.Tx.Tx
+ rg, readValErr := readAndValidateJsonStruct(r)
+ if readValErr != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil)
+ return
+ }
+
+ requestedRegionId := inf.IntParams["id"]
+ // check if the entity was already updated
+ userErr, sysErr, errCode = api.CheckIfUnModified(r.Header, inf.Tx, requestedRegionId, "region")
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+
+ query := `UPDATE region SET division= $1, name= $2 WHERE id= $3
+ RETURNING
+ id,last_updated,
+ (SELECT d.name FROM division d WHERE d.id = region.division)`
+
+ err := tx.QueryRow(query, rg.Division, rg.Name, requestedRegionId).Scan(&rg.ID, &rg.LastUpdated, &rg.DivisionName)
+ if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("region: %d not found", requestedRegionId), nil)
+ return
+ }
+ usrErr, sysErr, code := api.ParseDBError(err)
+ api.HandleErr(w, r, tx, code, usrErr, sysErr)
+ return
+ }
+
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region was updated")
+ api.WriteAlertsObj(w, r, http.StatusOK, alerts, rg)
+ return
+}
+
+// Delete an Region for APIv5
+func Delete(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ requestedRegionId := inf.Params["id"]
+ if requestedRegionId == "" {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("no region exists for empty id"), nil)
+ return
+ }
+
+ var exists bool
+ existErr := tx.QueryRow(`SELECT EXISTS(SELECT * from region where id = $1)`, requestedRegionId).Scan(&exists)
+
+ if existErr != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if region with id %s exists", existErr, requestedRegionId))
+ return
+ }
+ if !exists {
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("no region exists by id: %s", requestedRegionId), nil)
+ return
+ }
+
+ res, err := tx.Exec("DELETE FROM region WHERE id=$1", requestedRegionId)
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
+ return
+ }
+ rowsAffected, err := res.RowsAffected()
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("determining rows affected for delete region: %w", err))
+ return
+ }
+ if rowsAffected == 0 {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("no rows deleted for region"), nil)
+ return
+ }
+ alerts := tc.CreateAlerts(tc.SuccessLevel, "region was deleted.")
+ api.WriteAlerts(w, r, http.StatusOK, alerts)
+ return
+}
+
+// readAndValidateJsonStruct reads json body and validates json fields
+func readAndValidateJsonStruct(r *http.Request) (tc.RegionV5, error) {
+ var region tc.RegionV5
+ if err := json.NewDecoder(r.Body).Decode(®ion); err != nil {
+ userErr := fmt.Errorf("error decoding POST request body into RegionV5 struct %w", err)
+ return region, userErr
+ }
+
+ // validate JSON body
+ errs := tovalidate.ToErrors(validation.Errors{
+ "division": validation.Validate(region.Division, validation.NotNil, validation.Min(0)),
+ "name": validation.Validate(region.Name, validation.Required),
+ })
+ if len(errs) > 0 {
+ userErr := util.JoinErrs(errs)
+ return region, userErr
+ }
+ return region, nil
+}
+
+// SelectMaxLastUpdatedQuery used for TryIfModifiedSinceQuery()
+func SelectMaxLastUpdatedQuery(where string) string {
+ return `SELECT max(t) from (
+ SELECT max(r.last_updated) as t FROM region r
+JOIN division d ON r.division = d.id ` + where +
+ ` UNION ALL
+ select max(last_updated) as t from last_deleted l where l.table_name='region') as res`
Review Comment:
no it is not commented
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#issuecomment-1671749772
> Approved
I'll wait for the checks to pass, then merge
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] github-code-scanning[bot] commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1284612025
##########
traffic_ops/traffic_ops_golang/region/regions.go:
##########
@@ -192,3 +199,229 @@
query := deleteQueryBase() + ` WHERE id=:id`
return query
}
+
+// Read is the handler for GET requests to Regions of APIv5
+func Read(w http.ResponseWriter, r *http.Request) {
+ var maxTime time.Time
+ var runSecond bool
+
+ inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx
+ if sysErr != nil || userErr != nil {
+ api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ // Query Parameters to Database Query column mappings
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "division": {Column: "r.division"},
+ "id": {Column: "r.id", Checker: api.IsInt},
+ "name": {Column: "r.name"},
+ }
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "name"
+ }
+ where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
+ if len(errs) > 0 {
+ api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, util.JoinErrs(errs), nil)
+ }
+ if inf.Config.UseIMS {
+ runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, r.Header, queryValues, SelectMaxLastUpdatedQuery(where))
+ if !runSecond {
+ log.Debugln("IMS HIT")
+ api.AddLastModifiedHdr(w, maxTime)
+ w.WriteHeader(http.StatusNotModified)
+ return
+ }
+ log.Debugln("IMS MISS")
+ } else {
+ log.Debugln("Non IMS request")
+ }
+
+ query := selectQuery() + where + orderBy + pagination
+ rows, err := tx.NamedQuery(query, queryValues)
Review Comment:
## Database query built from user-controlled sources
This query depends on a [user-provided value](1).
[Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/298)
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1284717758
##########
traffic_ops/traffic_ops_golang/region/regions.go:
##########
@@ -20,14 +20,21 @@ package region
*/
import (
+ "database/sql"
+ "encoding/json"
"errors"
"fmt"
"net/http"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-tc/tovalidate"
+ "github.com/apache/trafficcontrol/lib/go-util"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
+ validation "github.com/go-ozzo/ozzo-validation"
Review Comment:
There should be a line break between Traffic Control imports and third-party imports.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7698: TO Fixes Region V5 apis to respond with RFC3339 date/time Format
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7698:
URL: https://github.com/apache/trafficcontrol/pull/7698#discussion_r1286134757
##########
traffic_ops/testing/api/v5/regions_test.go:
##########
@@ -298,7 +294,7 @@ func validateRegionsDescSort() utils.CkReqFunc {
func validateRegionsPagination(paginationParam string) utils.CkReqFunc {
return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, _ tc.Alerts, _ error) {
- paginationResp := resp.([]tc.Region)
+ paginationResp := resp.([]tc.RegionV5)
Review Comment:
This line panics for me. You're typecasting to `[]tc.RegionV5`, which is not the return type of the `Response` field:
https://github.com/apache/trafficcontrol/blob/0d79728bbd9d12e8464a294bb33409ca2282c68c/lib/go-tc/regions.go#L44-L45
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org