relates to C2-2197: setup the command serialization with tests #7
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package io.stargate.sgv3.docsapi.api.configuration; | ||
|
|
||
| import com.fasterxml.jackson.databind.MapperFeature; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.fasterxml.jackson.databind.json.JsonMapper; | ||
| import io.quarkus.jackson.ObjectMapperCustomizer; | ||
| import javax.enterprise.inject.Instance; | ||
| import javax.enterprise.inject.Produces; | ||
| import javax.inject.Singleton; | ||
|
|
||
| /** Configures the {@link ObjectMapper} instance that going to be injectable and used in the app. */ | ||
| public class ObjectMapperConfiguration { | ||
|
|
||
| /** Replaces the CDI producer for ObjectMapper built into Quarkus. */ | ||
| @Singleton | ||
| @Produces | ||
| ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) { | ||
| ObjectMapper mapper = createMapper(); | ||
|
|
||
| // apply all ObjectMapperCustomizer beans (incl. Quarkus) | ||
| for (ObjectMapperCustomizer customizer : customizers) { | ||
| customizer.customize(mapper); | ||
| } | ||
|
|
||
| return mapper; | ||
| } | ||
|
|
||
| private ObjectMapper createMapper() { | ||
| // enabled: | ||
| // case insensitive enums, so "before" will match to "BEFORE" in an enum | ||
| return JsonMapper.builder().enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS).build(); | ||
|
There was a problem hiding this comment. Do we really want to allow case-insensitive enum values? (I guess question for Aaron) There was a problem hiding this comment. I added it for the returnDocument option of the findOneAndUpdate command. While the enum name would be BEFORE or AFTER we would not want to force that in the JSON encoding There was a problem hiding this comment. One thing to note: instead of globally allowing all There was a problem hiding this comment. But then you must no forget it on every enum, globally it is imo.. There was a problem hiding this comment. Yes, true. This would be only if a small number of places allowed this which is sometimes the case. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties; | ||
| import com.fasterxml.jackson.annotation.JsonSubTypes; | ||
| import com.fasterxml.jackson.annotation.JsonTypeInfo; | ||
| import io.stargate.sgv3.docsapi.api.model.command.impl.FindOneCommand; | ||
| import io.stargate.sgv3.docsapi.api.model.command.impl.InsertOneCommand; | ||
|
|
||
| /** | ||
| * POJO object (data no behavior) that represents a syntactically and grammatically valid command as | ||
| * defined in the API spec. | ||
| * | ||
| * <p>The behavior about *how* to run a Command is in the {@link CommandResolver}. | ||
| * | ||
| * <p>Commands <b>should not</b> include JSON other than for documents we want to insert. They | ||
| * should represent a translate of the API request into an internal representation. e.g. this | ||
| * insulates from tweaking JSON on the wire protocol, we would only need to modify how we create the | ||
| * command and nothing else. | ||
| * | ||
| * <p>These may be created from parsing the incoming message and could also be created | ||
| * programmatically for internal and testing purposes. | ||
| * | ||
| * <p>Each command should validate itself using the <i>javax.validation</i> framework. | ||
| */ | ||
| @JsonTypeInfo( | ||
| use = JsonTypeInfo.Id.NAME, | ||
| include = JsonTypeInfo.As.WRAPPER_OBJECT, | ||
| property = "commandName") | ||
| @JsonSubTypes({ | ||
| @JsonSubTypes.Type(value = FindOneCommand.class), | ||
| @JsonSubTypes.Type(value = InsertOneCommand.class), | ||
| }) | ||
| @JsonIgnoreProperties({"commandName"}) | ||
|
There was a problem hiding this comment. I'll test this at a later point but I don't think this ignoral is needed. There was a problem hiding this comment. Yes you are correct, simply copied from the poc code and did not give a lot of thought here.. |
||
| public interface Command {} | ||
|
There was a problem hiding this comment. This should be called There was a problem hiding this comment. Can we put that after Command so we have a common base for all Commands. There was a problem hiding this comment. Yea would need to define the hierarchy then on the new class, keeping it like this, we can adapt when we add the |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command; | ||
|
|
||
| /** Base for any commands that modify, such as insert, delete, update, etc. */ | ||
| public interface ModifyCommand extends Command {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command; | ||
|
|
||
| public interface ReadCommand extends Command {} | ||
ivansenic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command.clause.sort; | ||
|
|
||
| import com.fasterxml.jackson.databind.annotation.JsonDeserialize; | ||
| import io.stargate.sgv3.docsapi.api.model.command.deserializers.SortClauseDeserializer; | ||
| import java.util.List; | ||
| import javax.validation.Valid; | ||
|
|
||
| /** | ||
| * Internal model for the sort clause that can be used in the commands. | ||
| * | ||
| * @param sortExpressions Ordered list of sort expressions. | ||
| */ | ||
| @JsonDeserialize(using = SortClauseDeserializer.class) | ||
| public record SortClause(@Valid List<SortExpression> sortExpressions) {} | ||
|
There was a problem hiding this comment. This needs proper schema defining this is a array of strings in the swagger There was a problem hiding this comment. I have been wondering about this: I thought the intent is for "DocsV3" to be backend for Mongoose drivers but not a general-purpose REST API. If so, would Swagger even be needed/useful? There was a problem hiding this comment. Yes for the target is not HTTP REST. We also want to avoid putting the JSON encoding attribution on the commands. The commands should just represent the object model, serialisation is seperate. There was a problem hiding this comment. Here there are few things that we should discuss:
There was a problem hiding this comment. @tatu-at-datastax @amorton Aaron and I kinda agreed to keep the swagger UI for now and try to define the schemas correctly.. We would not only have the benefit of supporting astra-prod requirements, but also gain benefits from the open API spec that can be used in postman, to generate clients or display docs in redoc or other similar tools.. Regarding the JSON encoding attribution, we agreed that we need to think for a day or two about extracting engine out of the http stuff. This would mean having two projects in this repo, one pure engine, another HTTP wrapper around.. IF we do so, the JSON encoding attribution would be done in the HTTP project using mixins. There was a problem hiding this comment. Yeah I am not dead against Swagger, but also want to help avoid significant effort being spent on something that won't be needed (if that's the case). There was a problem hiding this comment. On JSON bindings: to me most of annotation metadata is not a form of tight coupling. There are exceptions (specifying custom (de)serializers f.ex), but for basic external naming of properties where needed (overrides with |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command.clause.sort; | ||
|
|
||
| import javax.validation.constraints.NotBlank; | ||
|
|
||
| public record SortExpression( | ||
|
|
||
| // TODO correct typing for the path, we could use some kind of a common class | ||
| // we also need a validation of a correct path here | ||
| @NotBlank String path, | ||
|
|
||
| // this can be modeled in different ways, would this be enough for now | ||
| boolean ascending) {} | ||
|
Comment on lines
+7
to
+12
There was a problem hiding this comment. @amorton Look for suggestions here There was a problem hiding this comment. We cannot implement sort until we get sort in the core DB next year so I had not thought about it. But this is basically it, there is a list of field and sort order. There was a problem hiding this comment. Keep like this until further notice.. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command.deserializers; | ||
|
|
||
| import com.fasterxml.jackson.core.JacksonException; | ||
| import com.fasterxml.jackson.core.JsonParser; | ||
| import com.fasterxml.jackson.databind.DeserializationContext; | ||
| import com.fasterxml.jackson.databind.JsonMappingException; | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.deser.std.StdDeserializer; | ||
| import io.stargate.sgv3.docsapi.api.model.command.clause.sort.SortClause; | ||
| import io.stargate.sgv3.docsapi.api.model.command.clause.sort.SortExpression; | ||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| /** {@link StdDeserializer} for the {@link SortClause}. */ | ||
| public class SortClauseDeserializer extends StdDeserializer<SortClause> { | ||
|
|
||
| /** No-arg constructor explicitly needed. */ | ||
| public SortClauseDeserializer() { | ||
| super(SortClause.class); | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override | ||
| public SortClause deserialize(JsonParser parser, DeserializationContext ctxt) | ||
| throws IOException, JacksonException { | ||
| JsonNode node = parser.getCodec().readTree(parser); | ||
|
There was a problem hiding this comment. I can fix this, but better version is |
||
|
|
||
| // if missing or null, return null back | ||
| if (node.isMissingNode() || node.isNull()) { | ||
| return null; | ||
| } | ||
|
|
||
| // TODO, should we have specific exceptions, or throw generic JSON ones? | ||
|
There was a problem hiding this comment. @amorton wdyt, can you have a look on our There was a problem hiding this comment. I kept all the exceptions as RuntimeException to be consistency wrong as I was not aware how we wanted to model them. We will need something like that, I would guess there are Syntax errors and InvalidRequest for this sort of area, based on the CQL error states. There was a problem hiding this comment. Let's discuss and agree on this.. There was a problem hiding this comment. Not being discussed, will create an issue for this. There was a problem hiding this comment. I think that there absolutely should be at least one custom exception (base) type beyond JDK provided ones. Can start with a single "document exception" type which can be |
||
|
|
||
| // otherwise, if it's not array throw exception | ||
| if (!node.isArray()) { | ||
| throw new JsonMappingException( | ||
| parser, "Sort clause must be submitted as the array of strings."); | ||
| } | ||
|
|
||
| // safe iterate, we know it's array | ||
| List<SortExpression> expressions = new ArrayList<>(); | ||
| for (JsonNode inner : node) { | ||
| if (!inner.isTextual()) { | ||
| throw new JsonMappingException( | ||
| parser, "Sort clause expression must be represented as strings."); | ||
| } | ||
|
|
||
| String text = inner.asText(); | ||
| if (text.isBlank()) { | ||
| throw new JsonMappingException( | ||
| parser, "Sort clause expression must be represented as not-blank strings."); | ||
| } | ||
|
|
||
| boolean ascending = text.charAt(0) != '-'; | ||
| String path = ascending ? text : text.substring(1); | ||
| if (path.isBlank()) { | ||
| throw new JsonMappingException(parser, "A sort clause expression path must not be blank."); | ||
| } | ||
|
|
||
| SortExpression exp = new SortExpression(path.trim(), ascending); | ||
| expressions.add(exp); | ||
| } | ||
|
|
||
| return new SortClause(expressions); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command.impl; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.fasterxml.jackson.annotation.JsonTypeName; | ||
| import io.stargate.sgv3.docsapi.api.model.command.ReadCommand; | ||
| import io.stargate.sgv3.docsapi.api.model.command.clause.sort.SortClause; | ||
| import javax.validation.Valid; | ||
| import org.eclipse.microprofile.openapi.annotations.media.Schema; | ||
|
|
||
| @Schema(description = "Command that finds a single JSON document from a collection.") | ||
|
There was a problem hiding this comment. I'm a -1 on swagger annotations like this in the code. We want the Doc API library to be a library like the CQL Native Binary driver, it's with a HTTP/JSON interface is just one use. There could also be gRPC or included as a library in a client code. I have the same thoughts on sprint boot attributions like @Valid. It limits the library to working in a spring boot environment, which I would like to avoid for core business logic like this. There was a problem hiding this comment. Again, not sure I am following that you want not to have swagger? And how would this work with astra-prod? I see a benefit of having it, because you can simply start the app, go to swagger and start executing commands.. You don't need postman, collections, client, nothing.. But again let's discuss and agree.. There was a problem hiding this comment. Agreed that swagger annotations in the code make sense since they force programmer to change docs if the code changes.. It also ensures docs and code are merged together.. If we decide to split the engine in the separate project, then we need to see where should the annotation reside. |
||
| @JsonTypeName("findOne") | ||
| public record FindOneCommand(@Valid @JsonProperty("sort") SortClause sortClause) | ||
| implements ReadCommand {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package io.stargate.sgv3.docsapi.api.model.command.impl; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonTypeName; | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import io.stargate.sgv3.docsapi.api.model.command.Command; | ||
| import io.stargate.sgv3.docsapi.api.model.command.ModifyCommand; | ||
| import javax.validation.constraints.NotNull; | ||
| import org.eclipse.microprofile.openapi.annotations.enums.SchemaType; | ||
| import org.eclipse.microprofile.openapi.annotations.media.Schema; | ||
|
|
||
| /** | ||
| * Representation of the insertOne API {@link Command}. | ||
| * | ||
| * @param document The document to insert. | ||
| */ | ||
| @Schema(description = "Command that inserts a single JSON document to a collection.") | ||
| @JsonTypeName("insertOne") | ||
| public record InsertOneCommand( | ||
| @NotNull | ||
|
There was a problem hiding this comment. this should contain a proper error message.. There was a problem hiding this comment. We need to put the validate() methods back and do validation in there, for a couple of reasons.
There was a problem hiding this comment. You can achieve anything you want as you can easily create your own annotations, validations and rules. This had nothing to do with spring boot, this is therwise, you can avoid having it, but then create you own validation framework, meaning you need to have vlidations in place, you need a way to validate those, create error messages and translate that to the response with errors.. Wouldn't this be an overkill, especially when we are so fragile about the time? There was a problem hiding this comment. Agreed to keep the |
||
| @Schema( | ||
| description = "JSON document to insert.", | ||
| implementation = Object.class, | ||
| type = SchemaType.OBJECT) | ||
| JsonNode document) | ||
| implements ModifyCommand {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,56 @@ | ||
| package io.stargate.sgv3.docsapi.api.v3; | ||
|
|
||
| import io.smallrye.mutiny.Uni; | ||
| import io.stargate.sgv3.docsapi.api.model.command.Command; | ||
| import io.stargate.sgv3.docsapi.api.model.command.impl.FindOneCommand; | ||
| import io.stargate.sgv3.docsapi.api.model.command.impl.InsertOneCommand; | ||
| import io.stargate.sgv3.docsapi.config.constants.OpenApiConstants; | ||
| import javax.validation.Valid; | ||
| import javax.validation.constraints.NotNull; | ||
| import javax.ws.rs.Consumes; | ||
| import javax.ws.rs.POST; | ||
| import javax.ws.rs.Path; | ||
| import javax.ws.rs.Produces; | ||
| import javax.ws.rs.core.MediaType; | ||
| import org.eclipse.microprofile.openapi.annotations.Operation; | ||
| import org.eclipse.microprofile.openapi.annotations.media.Content; | ||
| import org.eclipse.microprofile.openapi.annotations.media.ExampleObject; | ||
| import org.eclipse.microprofile.openapi.annotations.media.Schema; | ||
| import org.eclipse.microprofile.openapi.annotations.parameters.Parameter; | ||
| import org.eclipse.microprofile.openapi.annotations.parameters.Parameters; | ||
| import org.eclipse.microprofile.openapi.annotations.parameters.RequestBody; | ||
| import org.eclipse.microprofile.openapi.annotations.security.SecurityRequirement; | ||
| import org.eclipse.microprofile.openapi.annotations.tags.Tag; | ||
| import org.jboss.resteasy.reactive.RestResponse; | ||
|
|
||
| @Path(CollectionResource.BASE_PATH) | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| @Consumes(MediaType.APPLICATION_JSON) | ||
| @SecurityRequirement(name = OpenApiConstants.SecuritySchemes.TOKEN) | ||
| @Tag(name = "Documents", description = "Executes document commands against a single collection.") | ||
| public class CollectionResource { | ||
|
|
||
| public static final String BASE_PATH = "/v3/{database}/{collection}"; | ||
|
|
||
| @Operation( | ||
| summary = "Execute command", | ||
| description = "Executes a single command against a collection.") | ||
| @Parameters( | ||
| value = { | ||
| @Parameter(name = "database", ref = "database"), | ||
| @Parameter(name = "collection", ref = "collection") | ||
| }) | ||
| @RequestBody( | ||
|
There was a problem hiding this comment. A Question - I quickly looked at the OpenAPI spec when I was looking at postman, and it looked like we would need to define the whole query API was that a correct assumption ? Happy for this to be here, we will not be promoting the Doc V3 API as a HTTP API to start with. To start with will be promoting and expecting all calls to come from Mongoose, I'm sure we will later expand to other ORM's and direct calls. There was a problem hiding this comment. I did not get this one sorry.. |
||
| content = | ||
| @Content( | ||
| mediaType = MediaType.APPLICATION_JSON, | ||
| schema = @Schema(anyOf = {FindOneCommand.class, InsertOneCommand.class}), | ||
| examples = { | ||
| @ExampleObject(ref = "findOne"), | ||
| @ExampleObject(ref = "insertOne"), | ||
| })) | ||
| @POST | ||
| public Uni<RestResponse<?>> postCommand(@NotNull @Valid Command command) { | ||
| return Uni.createFrom().item(RestResponse.ok()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the bigger naming question but... Isn't it bit confusing to have 2 completely different "Docs API" instances? If we do decide to go with "DocsV3" then at least add V3 in the name here, to tell from our existing "DocsV2".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package is different, would this be enough? I am happy to hear the suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's different for many uses. Not a big deal but I'd go with just
StargateDocsV3Api. Bit of redundancy in just one place.I do not propose to use it everywhere, just here.