Fixed TO API /servers/{id}/deliveryservices endpoint to respond with all DS's on cache that are directly assigned and inherited through topology. #7520

jagan-parthiban · 2023-05-15T05:19:01Z

Closes: #7519

Which Traffic Control components are affected by this PR?

Documentation
Traffic Ops

What is the best way to verify this PR?

Get request to Traffic Ops API /servers/{id}/deliveryservices endpoint should provide both DS that are directly assigned and that are inherited from the topology.

curl --request GET \
  --url https://localhost:8443/api/4.1/servers/{id}/deliveryservices

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

7.0.1

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)

docs/source/api/v4/servers_id_deliveryservices.rst

codecov · 2023-05-22T20:36:15Z

Codecov Report

Merging #7520 (c85c093) into master (daa7d21) will decrease coverage by 2.88%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #7520      +/-   ##
============================================
- Coverage     33.31%   30.44%   -2.88%     
  Complexity       98       98              
============================================
  Files           499      792     +293     
  Lines         50832    82868   +32036     
  Branches        851      885      +34     
============================================
+ Hits          16937    25227    +8290     
- Misses        32655    55527   +22872     
- Partials       1240     2114     +874

Flag	Coverage Δ
golib_unit	`48.56% <ø> (?)`
grove_unit	`4.60% <ø> (?)`
t3c_unit	`5.32% <ø> (?)`
traffic_monitor_unit	`21.35% <ø> (?)`
traffic_ops_integration	`69.42% <ø> (ø)`
traffic_ops_unit	`23.42% <0.00%> (-0.02%)`	⬇️
traffic_portal_v2	`74.93% <ø> (-0.06%)`	⬇️
traffic_stats_unit	`10.14% <ø> (?)`
unit_tests	`27.54% <0.00%> (-1.30%)`	⬇️
v3	`57.79% <ø> (ø)`
v4	`79.18% <ø> (ø)`
v5	`78.63% <ø> (ø)`

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

Impacted Files	Coverage Δ
...ffic_ops_golang/deliveryservice/servers/servers.go	`32.12% <0.00%> (-0.49%)`	⬇️

... and 306 files with indirect coverage changes

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

zrhoffman

Looks good, thanks!

zrhoffman

The V3 Delivery Service API tests fail:

--- FAIL: TestServersIDDeliveryServices (6.62s)
    --- FAIL: TestServersIDDeliveryServices/POST (0.13s)
        --- FAIL: TestServersIDDeliveryServices/POST/OK_when_VALID_request (0.03s)
            servers_id_deliveryservices_test.go:167: Not equal. Expected: 1 Actual: 2 Messages: Expected one Delivery Service returned Got: 2
        --- FAIL: TestServersIDDeliveryServices/POST/OK_when_ASSIGNING_ORIGIN_to_TOPOLOGY_BASED_DELIVERY_SERVICE (0.03s)
            servers_id_deliveryservices_test.go:167: Not equal. Expected: 1 Actual: 3 Messages: Expected one Delivery Service returned Got: 3
FAIL

Also, this PR needs to be rebased onto the master branch to get #7546, which will make CDN in a Box work again

jagan-parthiban · 2023-06-02T11:09:57Z

Fixed V3 Integration tests that were failing.

zrhoffman · 2023-06-02T15:57:44Z

traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go

 	}
-	where += "ds.id in (SELECT deliveryService FROM deliveryservice_server where server = :server)"
+
+	where += "ds.id in (SELECT deliveryService FROM deliveryservice_server WHERE server = :server) OR ds.id in (SELECT id FROM deliveryservice WHERE topology in ( SELECT topology FROM topology_cachegroup WHERE cachegroup = ( SELECT name FROM cachegroup WHERE id = ( SELECT cachegroup FROM server WHERE id = :server ))))"


In general, accomplishing this with JOINs for the

WHERE topology in ( SELECT topology FROM topology_cachegroup WHERE cachegroup = ( SELECT name FROM cachegroup WHERE id = ( SELECT cachegroup FROM server WHERE id = :server )))

instead of subqueries would be possible, but there is probably not a performance difference, so keeping this structure is fine.

zrhoffman · 2023-06-02T16:00:49Z

traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go

 	}
-	where += "ds.id in (SELECT deliveryService FROM deliveryservice_server where server = :server)"
+
+	where += "ds.id in (SELECT deliveryService FROM deliveryservice_server WHERE server = :server) OR ds.id in (SELECT id FROM deliveryservice WHERE topology in ( SELECT topology FROM topology_cachegroup WHERE cachegroup = ( SELECT name FROM cachegroup WHERE id = ( SELECT cachegroup FROM server WHERE id = :server ))))"


This is a long line for a query fragment. How about reformatting it as a multiline string like this?

where += ` ds.id in ( SELECT deliveryService FROM deliveryservice_server WHERE server = :server ) OR ds.id in ( SELECT id FROM deliveryservice WHERE topology in ( SELECT topology FROM topology_cachegroup WHERE cachegroup = ( SELECT name FROM cachegroup WHERE id = ( SELECT cachegroup FROM server WHERE id = :server )))) `

Fixed It. Thanks

zrhoffman · 2023-06-02T16:21:39Z

traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go

+		ds.id in (
+			SELECT deliveryService FROM deliveryservice_server WHERE server = :server
+		) OR ds.id in (
+			SELECT id FROM deliveryservice 
+			WHERE topology in ( 
+				SELECT topology FROM topology_cachegroup 
+				WHERE cachegroup = ( 
+					SELECT name FROM cachegroup 
+					WHERE id = ( 
+						SELECT cachegroup FROM server WHERE id = :server 
+					))))


Queries should not be indented like the rest of the function, because if you look at the complete, assembled query that Traffic Ops logs, that indentation does not make sense.

So, the query fragment should be unindented by 2 tabs.

zrhoffman

Looks good, thanks! I'll merge when the GHAs pass.

jagan-parthiban force-pushed the bug/deliveryservice-endpoint branch from 4ce1bf5 to 6a7e0aa Compare May 19, 2023 03:34

jagan-parthiban marked this pull request as ready for review May 19, 2023 17:57

ocket8888 reviewed May 22, 2023

View reviewed changes

docs/source/api/v4/servers_id_deliveryservices.rst Show resolved Hide resolved

zrhoffman added bug something isn't working as intended Traffic Ops related to Traffic Ops labels May 23, 2023

jagan-parthiban added 4 commits May 28, 2023 19:04

Fixes Issue: apache#7519

95f0c36

Changelog updated for Issue: apache#7519

5b297f5

Documentation updated for Issue: apache#7519

e413966

Updated Integration tests for apache#7519

bdd96b1

jagan-parthiban force-pushed the bug/deliveryservice-endpoint branch from a5dd2bc to bdd96b1 Compare May 28, 2023 13:38

CHANGELOG.md Update

10e2ff8

zrhoffman approved these changes Jun 1, 2023

View reviewed changes

zrhoffman suggested changes Jun 1, 2023

View reviewed changes

Updated V3 integration tests for this change.

bc5fedd

jagan-parthiban requested a review from zrhoffman June 2, 2023 11:16

zrhoffman suggested changes Jun 2, 2023

View reviewed changes

Fixed PR review comments

3d6e141

zrhoffman suggested changes Jun 2, 2023

View reviewed changes

Fixed PR review comments

c85c093

zrhoffman approved these changes Jun 2, 2023

View reviewed changes

zrhoffman merged commit fffe8a3 into apache:master Jun 2, 2023

jagan-parthiban deleted the bug/deliveryservice-endpoint branch June 8, 2023 10:50

asf-ci-trafficcontrol mentioned this pull request Jul 1, 2023

ATC Collaborators for July 2023 #7610

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed TO API /servers/{id}/deliveryservices endpoint to respond with all DS's on cache that are directly assigned and inherited through topology. #7520

Fixed TO API /servers/{id}/deliveryservices endpoint to respond with all DS's on cache that are directly assigned and inherited through topology. #7520

Uh oh!

jagan-parthiban commented May 15, 2023 •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented May 22, 2023 •

edited

Loading

Uh oh!

zrhoffman left a comment

Uh oh!

zrhoffman left a comment

Uh oh!

jagan-parthiban commented Jun 2, 2023

Uh oh!

zrhoffman Jun 2, 2023

Uh oh!

zrhoffman Jun 2, 2023

Uh oh!

jagan-parthiban Jun 2, 2023

Uh oh!

zrhoffman Jun 2, 2023

Uh oh!

jagan-parthiban Jun 2, 2023

Uh oh!

zrhoffman left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fixed TO API /servers/{id}/deliveryservices endpoint to respond with all DS's on cache that are directly assigned and inherited through topology. #7520

Fixed TO API /servers/{id}/deliveryservices endpoint to respond with all DS's on cache that are directly assigned and inherited through topology. #7520

Uh oh!

Conversation

jagan-parthiban commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zrhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

zrhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

jagan-parthiban commented Jun 2, 2023

Uh oh!

zrhoffman Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

zrhoffman Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

jagan-parthiban Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

zrhoffman Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

jagan-parthiban Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

zrhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jagan-parthiban commented May 15, 2023 •

edited

Loading

codecov bot commented May 22, 2023 •

edited

Loading