Add GET v3/service_offerings/:guid #3579

klapkov · 2024-10-30T16:10:49Z

Is there a related GitHub Issue?

#3578

What is this change about?

Here we add GetServiceOffering - GET v3/service_offerings/:guid

Does this PR introduce a breaking change?

no

Acceptance Steps

Tag your pair, your PM, and/or team

klapkov · 2024-10-30T16:13:54Z

api/handlers/service_offering_test.go

+		Describe("include broker field", func() {
+			BeforeEach(func() {
+				serviceBrokerRepo.ListServiceBrokersReturns([]repositories.ServiceBrokerRecord{{
+					ServiceBroker: services.ServiceBroker{
+						Name: "broker-name",
+					},
+					CFResource: model.CFResource{
+						GUID: "broker-guid",
+					},
+				}}, nil)


Not sure if we need this, since we already test the include broker field in the ListServiceOfferings test.

Yeah, looks a bit of a duplication. I would recommend having two whens: When("including broker name" and When("including broker guid that verify that proper broker fields are being returns. Error cases, as you have pointed out, are already covered in other tests.

After we use the generic include resolver as advised above testing becomes simpler too, for example

danail-branekov · 2024-10-31T15:19:17Z

api/payloads/service_offering_test.go

+		Entry("invalid service broker field", "fields[service_broker]=foo", MatchError(ContainSubstring("value must be one of: guid, name"))),
+	)
+
+	It("returns an error if a unsupported param is passed", func() {


That it could be an entry in the invalid query table above. Here is a similar example

danail-branekov · 2024-10-31T15:25:12Z

api/presenter/service_offering.go

@@ -54,3 +59,10 @@ func ForServiceOffering(serviceOffering repositories.ServiceOfferingRecord, base
 		},
 	}
 }
+
+func ForServiceOfferingWithIncluded(serviceOffering repositories.ServiceOfferingRecord, baseURL url.URL, includes ...model.IncludedResource) ServiceOfferingWithIncludedResponse {


I would suggest adding the Included field to the ServiceOfferingResponse type rather than coming up with a new one.

At first I took this approach, but this would mean that we would have to change the way it is handled in the list endpoint as well - https://github.com/cloudfoundry/korifi/blob/main/api/handlers/service_offering.go#L72. There we handle the "includes" with the ForList presenter and including it in the ForServiceOffering() would change ForList as well. Since only the LIST and GET endpoints for service offerings include fields, it did not seem right - https://v3-apidocs.cloudfoundry.org/version/3.180.0/index.html#fields-parameter. What do you think ?

Well, if the field is annotated with omitempty, then I believe it should be not visible in the response. If I am correct, then

the get handler would set it on the ServiceOfferingResponse object

the list handler would not, it would use the includes as ForList argument. As the Includes would not be set on the ServiceOfferingResponse object, they would be just omited in the response resources. TL;DR I believe that changing the list endpoint would not be necessary.

Or am I overlooking something?

danail-branekov · 2024-10-31T15:28:14Z

api/repositories/service_offering_repository.go

+
+	if err = userClient.Get(ctx, client.ObjectKeyFromObject(offering), offering); err != nil {
+		if k8serrors.IsForbidden(err) {
+			return ServiceOfferingRecord{}, nil


We should not be ignoring the forbidden error on Get. Instead, we should translate it to korifi forbidden error. You have actually done that below.
Lets just get rid of the forbidden check.

api/repositories/service_offering_repository_test.go

api/handlers/service_offering.go

georgethebeatle · 2024-11-05T14:00:18Z

api/handlers/service_offering.go

+		return nil, apierrors.LogAndReturn(logger, err, "failed to get service offering: %s", serviceOfferingGUID)
+	}
+
+	brokerIncludes, err := h.getBrokerIncludes(r.Context(), authInfo, []repositories.ServiceOfferingRecord{serviceOffering}, payload.IncludeBrokerFields, h.serverURL)


The generic include resolver can be used here, you can see how it works here and here

In this case should I replace the implementation on the list handler as well ?

georgethebeatle · 2024-11-05T14:03:59Z

api/handlers/service_offering_test.go

+		Describe("include broker field", func() {
+			BeforeEach(func() {
+				serviceBrokerRepo.ListServiceBrokersReturns([]repositories.ServiceBrokerRecord{{
+					ServiceBroker: services.ServiceBroker{
+						Name: "broker-name",
+					},
+					CFResource: model.CFResource{
+						GUID: "broker-guid",
+					},
+				}}, nil)


After we use the generic include resolver as advised above testing becomes simpler too, for example

georgethebeatle · 2024-11-05T14:06:20Z

api/payloads/service_offering.go

@@ -10,6 +10,25 @@ import (
 	jellidation "github.com/jellydator/validation"
 )

+type ServiceOfferingGet struct {
+	IncludeBrokerFields []string


We should be using the IncludeReourceRule type as we do for plans

Requested chages are done

klapkov commented Oct 30, 2024

View reviewed changes

klapkov force-pushed the get_offering branch from 64909f4 to 489f4d2 Compare October 30, 2024 18:45

danail-branekov reviewed Oct 31, 2024

View reviewed changes

danail-branekov previously requested changes Oct 31, 2024

View reviewed changes

georgethebeatle reviewed Nov 5, 2024

View reviewed changes

klapkov added 3 commits November 5, 2024 16:30

Implement GET v3/service_offerings/:guid

12f759b

Address comments

4c3ce18

Add includedResource to the itemPresenter interface

5166ac1

georgethebeatle force-pushed the get_offering branch from 3ac24cd to 5166ac1 Compare November 5, 2024 14:30

klapkov and others added 3 commits November 6, 2024 13:35

Use generic include resolver in GET handler

b363360

Merge branch 'main' into get_offering

a40ae8b

Add includes to stack presenter

d4edf9e

klapkov force-pushed the get_offering branch from 4d611a3 to d4edf9e Compare November 6, 2024 12:38

Use generic include resolver in LIST handler

c9ed5c2

georgethebeatle approved these changes Nov 8, 2024

View reviewed changes

Merge branch 'main' into get_offering

10bd8a5

georgethebeatle enabled auto-merge (squash) November 8, 2024 13:53

georgethebeatle linked an issue Nov 8, 2024 that may be closed by this pull request

API GET /v3/service_offerings/:guid is not implemented #3578

Closed

georgethebeatle merged commit 8394199 into cloudfoundry:main Nov 8, 2024
10 checks passed

klapkov mentioned this pull request Nov 18, 2024

[Feature]: Korifi API users can get a service offering by guid #2364

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GET v3/service_offerings/:guid #3579

Add GET v3/service_offerings/:guid #3579

klapkov commented Oct 30, 2024

klapkov Oct 30, 2024

danail-branekov Oct 31, 2024

klapkov Nov 1, 2024

georgethebeatle Nov 5, 2024

danail-branekov Oct 31, 2024

klapkov Nov 1, 2024

danail-branekov Oct 31, 2024

klapkov Nov 1, 2024

danail-branekov Nov 1, 2024 •

edited

Loading

danail-branekov Oct 31, 2024

klapkov Nov 1, 2024

georgethebeatle Nov 5, 2024

klapkov Nov 5, 2024

georgethebeatle Nov 5, 2024

georgethebeatle Nov 5, 2024

Add GET v3/service_offerings/:guid #3579

Add GET v3/service_offerings/:guid #3579

Conversation

klapkov commented Oct 30, 2024

Is there a related GitHub Issue?

What is this change about?

Does this PR introduce a breaking change?

Acceptance Steps

Tag your pair, your PM, and/or team

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danail-branekov Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danail-branekov Nov 1, 2024 •

edited

Loading