TO Fixes Region V5 apis to respond with RFC3339 date/time Format #7698

gbkannan89 · 2023-08-04T06:47:52Z

Related: #5911

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

This PR has tests
This PR has documentation
This PR has a CHANGELOG.md entry
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

codecov · 2023-08-04T16:11:39Z

Codecov Report

Merging #7698 (f93b368) into master (849d166) will decrease coverage by 0.08%.
The diff coverage is 3.47%.

@@             Coverage Diff              @@
##             master    #7698      +/-   ##
============================================
- Coverage     29.78%   29.70%   -0.08%     
  Complexity       98       98              
============================================
  Files           802      802              
  Lines         85789    86011     +222     
  Branches        952      952              
============================================
+ Hits          25550    25552       +2     
- Misses        58099    58320     +221     
+ Partials       2140     2139       -1

Flag	Coverage Δ
golib_unit	`48.05% <ø> (+<0.01%)`	⬆️
grove_unit	`4.60% <ø> (ø)`
t3c_unit	`4.95% <ø> (ø)`
traffic_monitor_unit	`21.30% <ø> (ø)`
traffic_ops_integration	`69.39% <100.00%> (ø)`
traffic_ops_unit	`22.16% <1.76%> (-0.13%)`	⬇️
traffic_portal_v2	`73.71% <ø> (+0.01%)`	⬆️
traffic_stats_unit	`10.14% <ø> (ø)`
unit_tests	`26.86% <1.76%> (-0.08%)`	⬇️
v3	`57.79% <ø> (ø)`
v4	`79.18% <ø> (ø)`
v5	`78.52% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
traffic_ops/traffic_ops_golang/region/regions.go	`7.48% <0.00%> (-14.84%)`	⬇️
traffic_ops/traffic_ops_golang/routing/routes.go	`95.53% <100.00%> (ø)`
traffic_ops/v5-client/region.go	`82.35% <100.00%> (ø)`

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

traffic_ops/traffic_ops_golang/region/regions.go

+	}
+
+	query := selectQuery() + where + orderBy + pagination
+	rows, err := tx.NamedQuery(query, queryValues)


zrhoffman

The API v5 tests fail:

    regions_test.go:324: Received unexpected error: error requesting Traffic Ops: path 'https://localhost:6443/api/5.0/regions' gave HTTP error 400 Bad Request - error-level alerts: error decoding POST request body into RegionV5 struct parsing time "\"0001-01-01 00:00:00+00\"" as "\"2006-01-02T15:04:05Z07:00\"": cannot parse " 00:00:00+00\"" as "T" Messages: Could not create Region 'region1': error requesting Traffic Ops: path 'https://localhost:6443/api/5.0/regions' gave HTTP error 400 Bad Request - error-level alerts: error decoding POST request body into RegionV5 struct parsing time "\"0001-01-01 00:00:00+00\"" as "\"2006-01-02T15:04:05Z07:00\"": cannot parse " 00:00:00+00\"" as "T" - alerts: [{Text:error decoding POST request body into RegionV5 struct parsing time "\"0001-01-01 00:00:00+00\"" as "\"2006-01-02T15:04:05Z07:00\"": cannot parse " 00:00:00+00\"" as "T" Level:error}]

the client still uses the the old Region struct:

trafficcontrol/traffic_ops/v5-client/region.go

Line 31 in 88faccc

func (to *Session) CreateRegion(region tc.Region, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) {
the fixtures still use the old Region struct:

trafficcontrol/traffic_ops/testing/api/v5/traffic_control_test.go

Line 46 in 88faccc

Regions []tc.Region `json:"regions"`

Both need to use the API v5 version instead.

zrhoffman · 2023-08-04T18:15:31Z

traffic_ops/traffic_ops_golang/region/regions.go

 	"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"


There should be a line break between Traffic Control imports and third-party imports.

zrhoffman · 2023-08-07T16:34:44Z

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)


This line panics for me. You're typecasting to []tc.RegionV5, which is not the type of the Response field:

trafficcontrol/lib/go-tc/regions.go

Lines 44 to 45 in 0d79728

type RegionsResponseV50 struct {

Response []Region `json:"response"`

kdamichie

Few changes needed.

kdamichie · 2023-08-07T15:50:14Z

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"                                                                                                       |


Missing ending pipe

kdamichie · 2023-08-07T15:55:09Z

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]_


Target "1" was removed, so his does not refer to anything anymore. Needs removed

kdamichie · 2023-08-07T15:56:42Z

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]_


Target "1" was removed, so his does not refer to anything anymore. Needs removed

kdamichie · 2023-08-07T16:45:07Z

traffic_ops/traffic_ops_golang/region/regions.go

+		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`


Any reason to keep this commented out statement?

no it is not commented

zrhoffman

Just 1 small comment.

zrhoffman · 2023-08-09T14:50:35Z

lib/go-tc/regions.go

+import "time"
+


Imports should be after the Apache license, not before.

zrhoffman

Looks good! But I'll wait for @kdamichie to approve before I merge.

kdamichie

Approved

docs/source/api/v5/regions.rst

zrhoffman · 2023-08-09T16:23:43Z

Approved

I'll wait for the checks to pass, then merge

…che#7698) * region v5 api * change log and doc * region comments addresed * response error fix * comments addressed * comment addressed * removed stray pipe --------- Co-authored-by: Zach Hoffman <zrhoffman@apache.org>

gbkannan89 added 3 commits August 4, 2023 11:57

region v5 api

9bef678

Merge branch 'master' into to-region-rfc3339

a79bc0b

change log and doc

88faccc

gbkannan89 marked this pull request as ready for review August 4, 2023 07:12

ericholguin added Traffic Ops related to Traffic Ops low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution labels Aug 4, 2023

github-advanced-security bot found potential problems Aug 4, 2023

View reviewed changes

traffic_ops/traffic_ops_golang/region/regions.go

}

query := selectQuery() + where + orderBy + pagination

rows, err := tx.NamedQuery(query, queryValues)

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources

This query depends on a [user-provided value](1).

zrhoffman requested changes Aug 4, 2023

View reviewed changes

gbkannan89 added 2 commits August 7, 2023 12:15

region comments addresed

514404d

Merge branch 'master' into to-region-rfc3339

0d79728

gbkannan89 requested a review from zrhoffman August 7, 2023 06:54

response error fix

d30fda2

zrhoffman requested changes Aug 7, 2023

View reviewed changes

kdamichie suggested changes Aug 7, 2023

View reviewed changes

gbkannan89 added 2 commits August 8, 2023 16:10

comments addressed

bbef04c

Merge branch 'master' into to-region-rfc3339

df0a3f0

gbkannan89 requested review from zrhoffman and kdamichie August 8, 2023 10:41

Merge branch 'master' into to-region-rfc3339

30f47e5

zrhoffman requested changes Aug 9, 2023

View reviewed changes

comment addressed

1c8f376

gbkannan89 requested a review from zrhoffman August 9, 2023 15:02

zrhoffman approved these changes Aug 9, 2023

View reviewed changes

removed stray pipe

f93b368

kdamichie approved these changes Aug 9, 2023

View reviewed changes

docs/source/api/v5/regions.rst Outdated Show resolved Hide resolved

Merge branch 'master' into to-region-rfc3339

52eb1c9

zrhoffman merged commit 6cab395 into apache:master Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TO Fixes Region V5 apis to respond with RFC3339 date/time Format #7698

TO Fixes Region V5 apis to respond with RFC3339 date/time Format #7698

gbkannan89 commented Aug 4, 2023

codecov bot commented Aug 4, 2023 •

edited

Loading

zrhoffman left a comment

zrhoffman Aug 4, 2023

zrhoffman Aug 7, 2023 •

edited

Loading

kdamichie left a comment

kdamichie Aug 7, 2023

kdamichie Aug 7, 2023

kdamichie Aug 7, 2023

kdamichie Aug 7, 2023

gbkannan89 Aug 8, 2023

zrhoffman left a comment

zrhoffman Aug 9, 2023

zrhoffman left a comment •

edited

Loading

kdamichie left a comment

zrhoffman commented Aug 9, 2023

	type RegionsResponseV50 struct {
	Response []Region `json:"response"`

		@@ -39,7 +39,6 @@ Request Structure
		+------+---------------------------------------------------------+

		:division: The new integral, unique identifier of the division which shall contain the region\ [1]_

		import "time"

TO Fixes Region V5 apis to respond with RFC3339 date/time Format #7698

TO Fixes Region V5 apis to respond with RFC3339 date/time Format #7698

Conversation

gbkannan89 commented Aug 4, 2023

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

codecov bot commented Aug 4, 2023 • edited Loading

Codecov Report

zrhoffman left a comment

Choose a reason for hiding this comment

zrhoffman Aug 4, 2023

Choose a reason for hiding this comment

zrhoffman Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

kdamichie left a comment

Choose a reason for hiding this comment

kdamichie Aug 7, 2023

Choose a reason for hiding this comment

kdamichie Aug 7, 2023

Choose a reason for hiding this comment

kdamichie Aug 7, 2023

Choose a reason for hiding this comment

kdamichie Aug 7, 2023

Choose a reason for hiding this comment

gbkannan89 Aug 8, 2023

Choose a reason for hiding this comment

zrhoffman left a comment

Choose a reason for hiding this comment

zrhoffman Aug 9, 2023

Choose a reason for hiding this comment

zrhoffman left a comment • edited Loading

Choose a reason for hiding this comment

kdamichie left a comment

Choose a reason for hiding this comment

zrhoffman commented Aug 9, 2023

codecov bot commented Aug 4, 2023 •

edited

Loading

zrhoffman Aug 7, 2023 •

edited

Loading

zrhoffman left a comment •

edited

Loading