PP-13030: Use CreateServiceRequest pojo instead of JsonNode #2653

oswaldquek · 2024-09-06T16:31:44Z

Following the general pattern in most of our Dropwizard codebases, we want to avoid JsonNode as the POST request body where possible so we can advantage of using javax Validation constraints.

It is also generally agreed that using hard coded URL strings makes for easier-to-read code.

Added a few more test cases in ServiceResourceCreateIT to reflect the pact tests between selfservice and adminusers:

oswaldquek · 2024-09-06T16:33:36Z

src/test/java/uk/gov/pay/adminusers/resources/ServiceResourceCreateIT.java

+                .body(Map.of("service_name", Map.of("fr", "Service name")))
+                .post("v1/api/services")
+                .then()
+                .statusCode(400)


Returning a 422 might be more correct here, but getting Jersey to do this from within a custom deserialiser is very hard to do. We return a 400 in other places so I think this is acceptable.

I agree this is fine for an internal API.

alexbishop1 · 2024-09-06T17:50:01Z

src/main/java/uk/gov/pay/adminusers/model/ServiceNamesDeserializer.java

+        Map<SupportedLanguage, String> supportedLanguageToServiceName = new HashMap<>();
+        jsonParser.getCodec().readValue(jsonParser, new TypeReference<Map<String, String>>() {})
+                .forEach((key, value) -> 
+                        supportedLanguageToServiceName.put(SupportedLanguage.fromIso639AlphaTwoCode(key), value));
+        return Collections.unmodifiableMap(supportedLanguageToServiceName);


return jsonParser.getCodec().readValue(jsonParser, new TypeReference<Map<String, String>>() {}) .entrySet().stream() .collect(toUnmodifiableMap(entry -> SupportedLanguage.fromIso639AlphaTwoCode(entry.getKey()), Map.Entry::getValue));

😀

Nice, fixed.

Following the general pattern in most of our Dropwizard codebases, we want to avoid JsonNode as the POST request body where possible so we can advantage of using javax Validation constraints. It is also generally agreed that using hard coded URL strings makes for easier-to-read code. Added a few more test cases in ServiceResourceCreateIT to reflect the pact tests between selfservice and adminusers: * [a valid create service request with empty object](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L50) * [a valid create service request with gateway account ids](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L85C31-L85C86) * [a valid create service request with service name](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L124)

alexbishop1

Looks good! Just a few small suggestions.

alexbishop1 · 2024-09-09T09:27:30Z

src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java

@@ -227,28 +220,21 @@ public Response searchServices(JsonNode payload) {
                    @ApiResponse(responseCode = "201", description = "Created",
                            content = @Content(schema = @Schema(implementation = Service.class))),
                    @ApiResponse(responseCode = "409", description = "Gateway account IDs provided has already been assigned to another service"),
+                    @ApiResponse(responseCode = "400", description = "Service names must be one of 'en' (English) or 'cy' (Welsh)"),


I suppose really it’s the service name key that needs to be ‘en’ or ‘cy’.

alexbishop1 · 2024-09-09T09:38:49Z

src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java

+        var nullableCreateServiceRequest = Optional.ofNullable(createServiceRequest);
+        Service service = serviceServicesFactory.serviceCreator().doCreate(
+                nullableCreateServiceRequest.map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()),
+                nullableCreateServiceRequest.map(CreateServiceRequest::serviceName).orElse(Map.of()));


Presumably the entire createServiceRequest is not going to be null but the list of gateway account IDs or map of languages to service names might be, so perhaps this would be better:

Service service = serviceServicesFactory.serviceCreator().doCreate( Optional.ofNullable(createServiceRequest.gatewayAccountIds()).orElse(List.of()), Optional.ofNullable(createServiceRequest.serviceName()).orElse(Map.of()));

createServiceRequest is null actually.... otherwise your suggestion would be the better option!

I think this would be easier:

Service service = serviceServicesFactory.serviceCreator().doCreate( createServiceRequest == null ? List.of() : createServiceRequest.gatewayAccountIds(), createServiceRequest == null ? Map.of() : createServiceRequest.serviceName());

But am not very fussed here.

⬆️ Actually my suggestion wouldn't work 🙈

Maybe this would work:

Service service = serviceServicesFactory.serviceCreator().doCreate( Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()), Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::serviceName).orElse(Map.of()));

It would get rid of the need to have the nullableCreateServiceRequest variable at the expense of an extra Optional.ofNullable(…) call (the cost of which is probably absolutely trivial).

This could be slightly rearranged to surface the actual data and types involved a bit more:

List<String> gatewayAccountIds = Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()); Map<SupportedLanguage, String> serviceNames = Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::serviceName).orElse(Map.of()); Service service = serviceServicesFactory.serviceCreator().doCreate(gatewayAccountIds, serviceNames);

alexbishop1 · 2024-09-09T09:39:54Z

src/test/java/uk/gov/pay/adminusers/resources/ServiceResourceCreateIT.java

+    private void assertStandardFields(ValidatableResponse validatableResponse) {
+        validatableResponse
+                .body("current_psp_test_account_stage", is("NOT_STARTED"))
+                .body("current_go_live_stage", is("NOT_STARTED"))
+                .body("default_billing_address_country", is("GB"))
+                .body("agent_initiated_moto_enabled", is(false))
+                .body("takes_payments_over_phone", is(false))
+                .body("experimental_features_enabled", is(false))
+                .body("internal", is(false))
+                .body("archived", is(false))
+                .body("redirect_to_service_immediately_on_terminal_state", is(false))
+                .body("collect_billing_address", is(true));
+    }


Maybe this method should go after all the tests that rely on it.

How do you mean? Like for other integration tests?

Or do you mean moving this method to the end of the class...

Yeah, at the end of the class or at least after all the tests that rely on it.

alexbishop1 · 2024-09-09T09:40:44Z

src/test/java/uk/gov/pay/adminusers/resources/ServiceResourceCreateIT.java

+                .body(Map.of("service_name", Map.of("fr", "Service name")))
+                .post("v1/api/services")
+                .then()
+                .statusCode(400)


I agree this is fine for an internal API.

alexbishop1

Looks fine. One take it or leave it suggestion.

alexbishop1 · 2024-09-09T11:40:57Z

src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java

+        var nullableCreateServiceRequest = Optional.ofNullable(createServiceRequest);
+        Service service = serviceServicesFactory.serviceCreator().doCreate(
+                nullableCreateServiceRequest.map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()),
+                nullableCreateServiceRequest.map(CreateServiceRequest::serviceName).orElse(Map.of()));


Maybe this would work:

Service service = serviceServicesFactory.serviceCreator().doCreate( Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()), Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::serviceName).orElse(Map.of()));

It would get rid of the need to have the nullableCreateServiceRequest variable at the expense of an extra Optional.ofNullable(…) call (the cost of which is probably absolutely trivial).

This could be slightly rearranged to surface the actual data and types involved a bit more:

List<String> gatewayAccountIds = Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()); Map<SupportedLanguage, String> serviceNames = Optional.ofNullable(createServiceRequest).map(CreateServiceRequest::serviceName).orElse(Map.of()); Service service = serviceServicesFactory.serviceCreator().doCreate(gatewayAccountIds, serviceNames);

oswaldquek · 2024-09-09T12:28:31Z

Looks fine. One take it or leave it suggestion.

I'll take it, but will put it in the next PR. Have another PR incoming for this repo and would like to get this one merged into master.

oswaldquek force-pushed the PP-13030-use-CreateServiceRequest-pojo branch from 4a4de0e to a3ed16d Compare September 6, 2024 16:32

oswaldquek commented Sep 6, 2024

View reviewed changes

oswaldquek force-pushed the PP-13030-use-CreateServiceRequest-pojo branch 2 times, most recently from da26a5e to bd365a8 Compare September 6, 2024 16:46

alexbishop1 reviewed Sep 6, 2024

View reviewed changes

oswaldquek added 2 commits September 9, 2024 10:25

use nicer collect style

9ac402e

oswaldquek force-pushed the PP-13030-use-CreateServiceRequest-pojo branch from 148f7ae to 9ac402e Compare September 9, 2024 09:25

add .secrets.baseline

e8469ed

alexbishop1 requested changes Sep 9, 2024

View reviewed changes

oswaldquek added 2 commits September 9, 2024 11:16

Clarify it's the service name keys

a2a8bc8

move private helper method

23e1f1d

alexbishop1 approved these changes Sep 9, 2024

View reviewed changes

oswaldquek merged commit 00a0f8a into master Sep 9, 2024
11 checks passed

oswaldquek deleted the PP-13030-use-CreateServiceRequest-pojo branch September 9, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP-13030: Use CreateServiceRequest pojo instead of JsonNode #2653

PP-13030: Use CreateServiceRequest pojo instead of JsonNode #2653

oswaldquek commented Sep 6, 2024 •

edited

Loading

oswaldquek Sep 6, 2024

alexbishop1 Sep 9, 2024

alexbishop1 Sep 6, 2024

oswaldquek Sep 9, 2024

alexbishop1 left a comment

alexbishop1 Sep 9, 2024

alexbishop1 Sep 9, 2024

oswaldquek Sep 9, 2024 •

edited

Loading

oswaldquek Sep 9, 2024

oswaldquek Sep 9, 2024

alexbishop1 Sep 9, 2024

alexbishop1 Sep 9, 2024

oswaldquek Sep 9, 2024

oswaldquek Sep 9, 2024

alexbishop1 Sep 9, 2024

alexbishop1 Sep 9, 2024

alexbishop1 left a comment

alexbishop1 Sep 9, 2024

oswaldquek commented Sep 9, 2024

PP-13030: Use CreateServiceRequest pojo instead of JsonNode #2653

PP-13030: Use CreateServiceRequest pojo instead of JsonNode #2653

Conversation

oswaldquek commented Sep 6, 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

alexbishop1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oswaldquek Sep 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbishop1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oswaldquek commented Sep 9, 2024

oswaldquek commented Sep 6, 2024 •

edited

Loading

oswaldquek Sep 9, 2024 •

edited

Loading