ACC-2678: REST body model#285
Conversation
7171400 to
b049f00
Compare
NielsCW
left a comment
There was a problem hiding this comment.
I first expected it to look more like restEntity in scribe defining all the endpoints. But it only defines the endpoints with a payload. Which is fine.
| * The resulting {@link ObjectBodyValue} can be used to derive OpenAPI request/response body schemas | ||
| * or HAL-Forms properties without re-reading the application model. | ||
| */ | ||
| public final class BodyObjectMapper { |
There was a problem hiding this comment.
Where does the name come from, shouldn't this be named BodyValueMapper?
There was a problem hiding this comment.
Well, it returns ObjectBodyValue, so it creates body objects? 🙂
| * @param userLocales the user's locales for translations | ||
| * @param entityName the entity to map | ||
| */ | ||
| public static ObjectBodyValue forSearch(Application application, UserLocales userLocales, EntityName entityName) { |
There was a problem hiding this comment.
Shouldn't this also contain the _sort query parameter with its options?
There was a problem hiding this comment.
I thought about that, but I decided not to do that, because:
- the
_sortfield is a REST-layer concern. Only the values there are used to create an object - The HAL-FORMS representation is more extended, having additional fields in the options that refer to the sort direction, the attribute to sort on and a translation. The
BodyValuestructure has nothing to cleanly map on that - Creating the sort parameter is quite straightforward, there is no difficult mapping to do there.
I agree that point 3 can also be said about the search parameters in general; but there I see more complexities (like handling hidden, the type of the search parameter and the constraints that apply to some search parameters and not others
| import lombok.NonNull; | ||
| import lombok.Value; | ||
|
|
||
| public sealed interface SourceType { |
There was a problem hiding this comment.
I have no clue what this does. Please add javadoc.
| if (attribute.hasFlag(IgnoredFlag.class)) { | ||
| return; | ||
| } | ||
| if (context.bodyType() != BodyType.RESPONSE && attribute.hasFlag(ReadOnlyFlag.class)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don't these belong in getBodyValue?
There was a problem hiding this comment.
getBodyValue() is also used in forSearch(); so for search operations.
I think we still want to be able to place a search filter on an ignored attribute?
Introduces com.contentgrid.appserver.application.model.openapi.model.rest.body with an abstract BodyValue class hierarchy (ObjectBodyValue, SimpleBodyValue, RelationBodyValue, ContentBodyValue, ArrayBodyValue) and a BodyObjectMapper that converts an Entity+Application to a typed key-value tree. BodyType (RESPONSE/POST/PUT/PATCH) and MediaType (JSON/FORM/MULTIPART_FORM) drive which attributes are included and how nullability is derived, allowing OpenAPI schemas and HAL-Forms templates to be generated from a single model.
b049f00 to
80d278b
Compare
It does not define endpoints at all; it only defines payloads actually. To be honest, also using it for search parameters was a bit of an afterthought; but I do think that it fits relatively nicely. |
| case UUID -> JsonSchemaFormat.UUID; | ||
| default -> null; | ||
| }; | ||
| private Stream<JsonSchemaProperty> createProperties(Context ctx, CreatePropertiesContext context, Definitions definitions) { |
There was a problem hiding this comment.
Two different Context arguments is really confusing. Both are defined in JsonSchemaAssembler anyway.
There was a problem hiding this comment.
I can rename the variables, or make the context nested.
But doing it without a separate context object for the CreatePropertiesContext makes it really difficult to read (both as additional methods or having the code inlined)
| .getNestedAttribute(attributeSourceType.getAttributePath()) | ||
| .filter(ContentAttribute.class::isInstance) | ||
| .isPresent(); | ||
| if (isContentAttribute) { |
There was a problem hiding this comment.
I'm not completely following here. For ContentBodyValues, you throw, but then you follow the path to see if it is a content property?
There was a problem hiding this comment.
ContentBodyValue is a specific type that indicates that it's a BodyValue where content can be put in directly (so a file-upload kind of thing)
That can't be represented in JSON (and thus also not in JSON schema). It's never returned when creating a BodyValue for MediaType JSON, but the switch needs to be exhaustive.
|
|
||
| var body = BodyObjectMapper.forBody(new Context(application, BodyType.POST, MULTIPART_FORM, userLocales), entityName); | ||
| var properties = toHalFormsProperties(body); | ||
| var hasFiles = properties.stream().anyMatch(p -> Objects.equals(HtmlInputType.FILE_VALUE, p.getType())); |
There was a problem hiding this comment.
Probably better to look up in the application/entity if it has content properties and then decide to use MULTIPART_FORM or JSON in BodyObjectMapper.forBody.
There was a problem hiding this comment.
This was already how it worked before actually; this is the part that has not changed
There was a problem hiding this comment.
Right, I know it worked like this before. It would just look more consistent if the value in BodyObjectMapper#forBody matches the value in HalFormsTemplate#contentType.
Replace ad-hoc attribute traversal in generateCreateTemplate, generateUpdateTemplate and generateSearchTemplate with the BodyObjectMapper intermediate model, eliminating the PrefixSettings pattern and duplicated traversal logic.
5d046fc to
0b43bbc
Compare
… generate the schema Refactor JsonSchemaAssembler to reuse BodyObjectMapper instead of duplicating entity/relation traversal.
0b43bbc to
0a76852
Compare
|
|
||
| var body = BodyObjectMapper.forBody(new Context(application, BodyType.POST, MULTIPART_FORM, userLocales), entityName); | ||
| var properties = toHalFormsProperties(body); | ||
| var hasFiles = properties.stream().anyMatch(prop -> Objects.equals(HtmlInputType.FILE_VALUE, prop.getType())); |
There was a problem hiding this comment.
Right, I know it worked like this before. It would just look more consistent if the value in BodyObjectMapper#forBody matches the value in HalFormsTemplate#contentType.
| var body = BodyObjectMapper.forBody(new Context(application, BodyType.PUT, FORM, userLocales), entityName); | ||
| var properties = toHalFormsProperties(body); |
There was a problem hiding this comment.
The content-type of the template is application/json, please use JSON in BodyObjectMapper#forBody for consistency.
There was a problem hiding this comment.
JSON results in a nested structure, where FORM is flattened.
HAL-FORMS results are flattened. I have added JSON_FLAT as a separate type that does support nulls, but still results in a flat structure. The resulting representation is identical to FORM, but it does convey the meaning better.
…ilters Exact search filters need to match the value of an attribute exactly, so it makes sense to apply the constraints to that search filter type as well
The HAL-FORMS representation does not support nested fields, to the fields need to be flattened there as well. The resulting representation is currently identical to `FORM`, but this makes the meaning more clear (and not just a side-effect of the selected mediatype).
392ceaa to
b9d029c
Compare
|



First part of creating the OpenAPI spec generator.
Create an intermediate model of request/response bodies, so HAL-FORMS, json schema and openapi spec don't each have their own view on them.
Creating one shared view on what the bodies look like will result in a single source of truth.