Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@
{
"path": "detect_secrets.filters.allowlist.is_line_allowlisted"
},
{
"path": "detect_secrets.filters.common.is_baseline_file",
"filename": ".secrets.baseline"
},
{
"path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
"min_level": 2
Expand Down Expand Up @@ -139,35 +135,35 @@
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "614770647df3ab100b871bfc0d20e72c8625a5c4",
"is_verified": false,
"line_number": 398
"line_number": 400
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "1976a945a8d2733655e4b2453bd49fb59cb7ba19",
"is_verified": false,
"line_number": 663
"line_number": 665
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "a31519136d26754afeb0df49b5f066f387091f88",
"is_verified": false,
"line_number": 697
"line_number": 699
},
{
"type": "Secret Keyword",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "0ea7458942ab65e0a340cf4fd28ca00d93c494f3",
"is_verified": false,
"line_number": 796
"line_number": 798
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "3660e32cde5fccc8d1e4521d0c831c2012388720",
"is_verified": false,
"line_number": 1231
"line_number": 1244
}
],
"src/main/java/uk/gov/pay/adminusers/model/CreateUserRequest.java": [
Expand Down Expand Up @@ -260,14 +256,14 @@
"filename": "src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java",
"hashed_secret": "614770647df3ab100b871bfc0d20e72c8625a5c4",
"is_verified": false,
"line_number": 149
"line_number": 142
},
{
"type": "Hex High Entropy String",
"filename": "src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java",
"hashed_secret": "1976a945a8d2733655e4b2453bd49fb59cb7ba19",
"is_verified": false,
"line_number": 383
"line_number": 369
}
],
"src/main/java/uk/gov/pay/adminusers/resources/ToolboxEndpointResource.java": [
Expand Down Expand Up @@ -466,5 +462,5 @@
}
]
},
"generated_at": "2024-09-06T15:07:58Z"
"generated_at": "2024-09-09T09:33:48Z"
}
13 changes: 13 additions & 0 deletions openapi/adminusers_spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ paths:
schema:
$ref: '#/components/schemas/Service'
description: Created
"400":
description: Service name keys must be one of 'en' (English) or 'cy' (Welsh)
"409":
description: Gateway account IDs provided has already been assigned to another
service
Expand Down Expand Up @@ -1184,6 +1186,17 @@ components:
example: [email protected]
required:
- email
CreateServiceRequest:
type: object
properties:
gateway_account_ids:
type: array
items:
type: string
service_name:
type: object
additionalProperties:
type: string
CreateUserRequest:
type: object
properties:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package uk.gov.pay.adminusers.model;

import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.util.List;
import java.util.Map;

@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public record CreateServiceRequest(
List<String> gatewayAccountIds,
@JsonDeserialize(using = ServiceNamesDeserializer.class) Map<SupportedLanguage, String> serviceName) {
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package uk.gov.pay.adminusers.model;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.io.IOException;
import java.util.Map;

import static java.util.stream.Collectors.toUnmodifiableMap;

public class ServiceNamesDeserializer extends JsonDeserializer<Map<SupportedLanguage, String>> {
@Override
public Map<SupportedLanguage, String> deserialize(JsonParser jsonParser, DeserializationContext deserializationContext) throws IOException {
return jsonParser.getCodec().readValue(jsonParser, new TypeReference<Map<String, String>>() {})
.entrySet().stream()
.collect(toUnmodifiableMap(entry -> SupportedLanguage.fromIso639AlphaTwoCode(entry.getKey()), Map.Entry::getValue));
}
}
44 changes: 9 additions & 35 deletions src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import uk.gov.pay.adminusers.exception.ServiceNotFoundException;
import uk.gov.pay.adminusers.model.CreateServiceRequest;
import uk.gov.pay.adminusers.model.GovUkPayAgreement;
import uk.gov.pay.adminusers.model.SearchServicesResponse;
import uk.gov.pay.adminusers.model.Service;
Expand All @@ -33,7 +34,6 @@
import uk.gov.pay.adminusers.service.StripeAgreementService;
import uk.gov.pay.adminusers.utils.Errors;
import uk.gov.service.payments.commons.api.exception.ValidationException;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import javax.validation.Valid;
import javax.validation.constraints.NotNull;
Expand All @@ -53,13 +53,9 @@
import java.net.UnknownHostException;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static java.util.stream.Collectors.toUnmodifiableList;
import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
Expand All @@ -71,15 +67,12 @@
import static javax.ws.rs.core.Response.Status.NO_CONTENT;
import static javax.ws.rs.core.Response.Status.OK;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static uk.gov.pay.adminusers.resources.ServiceResource.SERVICES_RESOURCE;
import static uk.gov.pay.adminusers.service.ServiceUpdater.FIELD_GATEWAY_ACCOUNT_IDS;

@Path(SERVICES_RESOURCE)
@Path("/v1/api/services")
public class ServiceResource {

private static final Logger LOGGER = LoggerFactory.getLogger(ServiceResource.class);
/* default */static final String HEADER_USER_CONTEXT = "GovUkPay-User-Context";
public static final String SERVICES_RESOURCE = "/v1/api/services";

public static final String FIELD_NAME = "name";

Expand Down Expand Up @@ -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 name keys must be one of 'en' (English) or 'cy' (Welsh)"),
@ApiResponse(responseCode = "500", description = "Invalid JSON payload")
}
)
public Response createService(JsonNode payload) {
LOGGER.info("Create Service POST request - [ {} ]", payload);
List<String> gatewayAccountIds = extractGatewayAccountIds(payload);
Map<SupportedLanguage, String> serviceNameVariants = getServiceNameVariants(payload);
public Response createService(@Valid CreateServiceRequest createServiceRequest) {
LOGGER.info("Create Service POST request - [ {} ]", createServiceRequest);

Service service = serviceServicesFactory.serviceCreator().doCreate(gatewayAccountIds, serviceNameVariants);
var nullableCreateServiceRequest = Optional.ofNullable(createServiceRequest);
Service service = serviceServicesFactory.serviceCreator().doCreate(
nullableCreateServiceRequest.map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()),
nullableCreateServiceRequest.map(CreateServiceRequest::serviceName).orElse(Map.of()));
Comment on lines +230 to +233
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

Copy link
Contributor Author

@oswaldquek oswaldquek Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬆️ Actually my suggestion wouldn't work 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

return Response.status(CREATED).entity(service).build();

}

private List<String> extractGatewayAccountIds(JsonNode payload) {
List<String> gatewayAccountIds = new ArrayList<>();
if (payload != null && payload.get(FIELD_GATEWAY_ACCOUNT_IDS) != null) {
payload.get(FIELD_GATEWAY_ACCOUNT_IDS)
.elements().forEachRemaining((node) -> gatewayAccountIds.add(node.textValue()));
}
return List.copyOf(gatewayAccountIds);
}

@Path("/{serviceExternalId}")
@PATCH
@Produces(APPLICATION_JSON)
Expand Down Expand Up @@ -533,16 +519,4 @@ public Response sendLiveAccountCreatedEmail(@Parameter(example = "7d19aff33f8948
sendLiveAccountCreatedEmailService.sendEmail(serviceExternalId);
return Response.status(OK).build();
}

private Map<SupportedLanguage, String> getServiceNameVariants(JsonNode payload) {
if (payload.hasNonNull("service_name")) {
JsonNode serviceName = payload.get("service_name");
return Stream.of(SupportedLanguage.values())
.filter(supportedLanguage -> serviceName.hasNonNull(supportedLanguage.toString()))
.collect(Collectors.toUnmodifiableMap(
supportedLanguage -> supportedLanguage,
supportedLanguage -> serviceName.get(supportedLanguage.toString()).asText()));
}
return Collections.emptyMap();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

import static javax.ws.rs.core.UriBuilder.fromUri;
import static uk.gov.pay.adminusers.resources.ForgottenPasswordResource.FORGOTTEN_PASSWORDS_RESOURCE;
import static uk.gov.pay.adminusers.resources.ServiceResource.SERVICES_RESOURCE;
import static uk.gov.pay.adminusers.resources.UserResource.USERS_RESOURCE;

public class LinksBuilder {
Expand All @@ -32,7 +31,7 @@ public User decorate(User user) {
}

public Service decorate(Service service) {
URI uri = fromUri(baseUrl).path(SERVICES_RESOURCE).path(String.valueOf(service.getExternalId()))
URI uri = fromUri(baseUrl).path("/v1/api/services").path(String.valueOf(service.getExternalId()))
.build();
Link selfLink = Link.from(Rel.SELF, "GET", uri.toString());
service.setLinks(List.of(selfLink));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public Response sendEmail() {
@BeforeEach
public void initialise() {
databaseHelper = APP.getDatabaseTestHelper();
databaseHelper.truncateAllData();
}

protected RequestSpecification givenSetup() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,117 @@
package uk.gov.pay.adminusers.resources;

import com.fasterxml.jackson.databind.JsonNode;
import io.restassured.response.ValidatableResponse;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.DisplayNameGenerator;
import org.junit.jupiter.api.Test;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.util.List;
import java.util.Map;

import static io.restassured.http.ContentType.JSON;
import static org.hamcrest.Matchers.emptyIterable;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static uk.gov.service.payments.commons.model.SupportedLanguage.ENGLISH;
import static uk.gov.service.payments.commons.model.SupportedLanguage.WELSH;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class ServiceResourceCreateIT extends IntegrationTest {

@Test
void shouldCreateService() {
JsonNode payload = mapper
.valueToTree(Map.of(
"service_name", Map.of(SupportedLanguage.ENGLISH.toString(), "Service name"),
"gateway_account_ids", List.of("1")
));

givenSetup()
void create_service_with_all_parameters_successfully() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(payload)
.body(Map.of("service_name", Map.of(ENGLISH.toString(), "Service name"),
"gateway_account_ids", List.of("1")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(List.of("1")))
.body("service_name", hasEntry("en", "Service name"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_empty_request_body() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(emptyIterable()))
.body("service_name", hasEntry("en", "System Generated"))
.body("name", is("System Generated"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_gateway_account_ids_only() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(Map.of("gateway_account_ids", List.of("1", "2")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(List.of("1", "2")))
.body("service_name", hasEntry("en", "System Generated"))
.body("name", is("System Generated"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_service_name_only() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(Map.of("service_name", Map.of(ENGLISH.toString(), "Service name", WELSH.toString(), "Welsh name")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(emptyIterable()))
.body("service_name", hasEntry("en", "Service name"))
.body("service_name", hasEntry("cy", "Welsh name"))
.body("name", is("Service name"));

assertStandardFields(validatableResponse);
}

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));
}

@Test
void return_bad_request_when_invalid_supported_language_provided() {
givenSetup()
.when()
.contentType(JSON)
.body(Map.of("service_name", Map.of("fr", "Service name")))
.post("v1/api/services")
.then()
.statusCode(400)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is fine for an internal API.

.body("message", is("Unable to process JSON"))
.body("details", is("fr is not a supported ISO 639-1 code"));
}
}
Loading
Loading