relates to C2-2197: setup the command serialization with tests #7

ivansenic · 2022-11-28T13:30:47Z

Work in progress..

This PR summarizes show we should pull the code from the POC/demo project @amorton created.. There are a lot of things that need attention, so let me try to explain it here:

commands
- modelling hierarchy does not need mixins; mixins are used when you need to de-serialize 3rd party entities, here we can have everything we want directly in the commands itself (copied everything from demo as is)
- the Command, ReadCommand and ModifyCommand are modeled as interfaces, mainly so we can use record for the command impl
  - makes them immutable and it's a perfect match for a native image (hopefully, seems there are issues with the validation)
  - interfaces have private methods in Java 17, so there is no diff from the abstract class
- removed the isValid and validate stuff, not needed as we can use javax.validation directly (see example on missing document node in insertOne)
(de-)serializers
- SortClause is simplest, so I went for implementing that in the deserializing
- we can annotate classes directly with @JsonDeserialize so no need for custom modules, again modules are usually coming from 3rd party libs
object mapper config done globally and correctly
api endpoint can serialize two defined commands and get a command interface
tests
- proper testing of de-serializer
- proper testing of the object mapper reading different commands
- proper int testing to ensure those same commands can be consumed by the api
- proper int testing to ensure commands are validated
swagger
- correctly define commands and examples, so they can be executed directly from swagger, please check the http://localhost:8080/swagger-ui

Missing peaces:

would like to continue with the modeling of the findOne command
optionally we could define the response type that the endpoints define, so we have two commands + response DTO done

ivansenic

Code comments..

ivansenic · 2022-11-28T15:35:36Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/Command.java

+  @JsonSubTypes.Type(value = InsertOneCommand.class),
+})
+@JsonIgnoreProperties({"commandName"})
+public interface Command {}


This should be called CollectionCommand as we need to differentiate between commands that can be used on the /v3/{database} and /v3/{database}/{collection}

Can we put that after Command so we have a common base for all Commands.

Yea would need to define the hierarchy then on the new class, keeping it like this, we can adapt when we add the DatabaseCommands.

src/main/java/io/stargate/sgv3/docsapi/api/model/command/ReadCommand.java

ivansenic · 2022-11-28T15:36:17Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/sort/SortClause.java

+ * @param sortExpressions Ordered list of sort expressions.
+ */
+@JsonDeserialize(using = SortClauseDeserializer.class)
+public record SortClause(@Valid List<SortExpression> sortExpressions) {}


This needs proper schema defining this is a array of strings in the swagger

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?

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.

Here there are few things that we should discuss:

do you both imply that we don't need swagger ui here? why is that, seems quite handy? how would this be applied to Astra prod where we do need the swagger ui?

avoid putting the JSON encoding attribution on the commands, what does this mean? is this for going back on the mixins way? so you would rather not have model and serialization in one place, easy to understand and look at, but rather split this into another, effectively not needed, class that you need to search and find to understand how's everything working?

@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.

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).
Sort of like why GraphQL doesn't have Swagger definitions even though technically speaking one could add definitions (but no one would use Swagger).

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 @JsonProperty) externalization seems like unnecessary work.
There's only a single dependency (jackson-annotations) package (which itself has no deps), for annotations themselves and any library/framework is free to use or not those (that is: there is no need to depend on Jackson library for parsing/generation of JSON at all; only annotation types). Model itself could be used with other libraries too (but possibly adding annotation/configuration they need).
I can help minimizing need for annotations and maybe avoid most or all mix-ins.

ivansenic · 2022-11-28T15:36:33Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/sort/SortExpression.java

+    // 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) {}


@amorton Look for suggestions here

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.

Keep like this until further notice..

ivansenic · 2022-11-28T15:37:05Z

...in/java/io/stargate/sgv3/docsapi/api/model/command/deserializers/SortClauseDeserializer.java

+      return null;
+    }
+
+    // TODO, should we have specific exceptions, or throw generic JSON ones?


@amorton wdyt, can you have a look on our BusinessRuntimeException idea in the v2?

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.

Let's discuss and agree on this..

Not being discussed, will create an issue for this.

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 RuntimeException if we want to avoid checked exceptions (which seems to be where most Java devs are leaning).

ivansenic · 2022-11-28T17:19:48Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/impl/InsertOneCommand.java

+@Schema(description = "Command that inserts a single JSON document to a collection.")
+@JsonTypeName("insertOne")
+public record InsertOneCommand(
+    @NotNull


this should contain a proper error message..

We need to put the validate() methods back and do validation in there, for a couple of reasons.

there will be complex validation steps, e.g. check that nothing in an update clause tries to change the _id of a document, or that the data types for an _id document are those we support.

we want the document library to work outside of a spring boot environment.

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 javax.validation, a framework that helps you validate things easier.. Meaning, that you can still use the framework outside of spring boot or quarkus.

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?

Agreed to keep the javax.validation. For now we will let Quarkus validate commands. In the future, we want to integrate this into engine, so that commands are validated no matter how are they created. We have a ticket for validation, so we will do it there: https://datastax.jira.com/browse/C2-2280

tatu-at-datastax · 2022-11-28T17:56:43Z

src/main/java/io/stargate/sgv3/docsapi/StargateDocsApi.java

+                          }
+                        }
+                      }
+                      """),
            }))
 public class StargateDocsApi extends Application {


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".

Package is different, would this be enough? I am happy to hear the suggestions?

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.

tatu-at-datastax · 2022-11-28T17:57:32Z

src/main/java/io/stargate/sgv3/docsapi/api/configuration/ObjectMapperConfiguration.java

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


Do we really want to allow case-insensitive enum values? (I guess question for Aaron)

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

One thing to note: instead of globally allowing all Enum values to be case-insensitive, it is possible to annotate just ones that do. It's with something like:

@JsonFormat(with = [ JsonFormat.Feature.ACCEPT_CASE_INSENSITIVE_VALUES ]) public MyEnum value; // on field or accessor/method

But then you must no forget it on every enum, globally it is imo..

Yes, true. This would be only if a small number of places allowed this which is sometimes the case.

tatu-at-datastax · 2022-11-28T17:58:10Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/Command.java

+  @JsonSubTypes.Type(value = FindOneCommand.class),
+  @JsonSubTypes.Type(value = InsertOneCommand.class),
+})
+@JsonIgnoreProperties({"commandName"})


I'll test this at a later point but I don't think this ignoral is needed.

Yes you are correct, simply copied from the poc code and did not give a lot of thought here..

tatu-at-datastax · 2022-11-28T18:03:30Z

...in/java/io/stargate/sgv3/docsapi/api/model/command/deserializers/SortClauseDeserializer.java

+  @Override
+  public SortClause deserialize(JsonParser parser, DeserializationContext ctxt)
+      throws IOException, JacksonException {
+    JsonNode node = parser.getCodec().readTree(parser);


I can fix this, but better version is

JsonNode node = ctxt.readTree(parser);

amorton · 2022-11-29T06:56:51Z

src/main/java/io/stargate/sgv3/docsapi/api/model/command/impl/FindOneCommand.java

+import javax.validation.Valid;
+import org.eclipse.microprofile.openapi.annotations.media.Schema;
+
+@Schema(description = "Command that finds a single JSON document from a collection.")


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.

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..

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.

amorton · 2022-11-29T07:02:52Z

src/main/java/io/stargate/sgv3/docsapi/api/v3/CollectionResource.java

+        @Parameter(name = "database", ref = "database"),
+        @Parameter(name = "collection", ref = "collection")
+      })
+  @RequestBody(


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.

I did not get this one sorry..

ivansenic · 2022-11-29T10:31:51Z

@tatu-at-datastax @amorton Added review fixes, added comments about the discussion points.. I would like to merge this as is and continue, but an approval.. The todos are left for future work..

tatu-at-datastax · 2022-11-29T22:17:34Z

I am +1 for fast merging, fixing things that need fixing later on.
But I'll let @amorton decide here.

ivansenic · 2022-11-30T13:40:49Z

Why not giving an approval here? Aaron did not provide one as well, and now I have to wait even longer?

tatu-at-datastax · 2022-11-30T18:02:52Z

Why not giving an approval here? Aaron did not provide one as well, and now I have to wait even longer?

Sorry I wasn't sure if Aaron had concerns to resolve (yes, I know that technically it is possible to add request for change).

But I'll approve this now (for tomorrow).

tatu-at-datastax

As per my notes, assuming @amorton has no objections.

ivansenic · 2022-12-01T16:04:34Z

Merging this, remaining comments can be pushed to #10

ivansenic requested a review from a team as a code owner November 28, 2022 13:30

ivansenic force-pushed the ise-c2-2197-dtos branch 2 times, most recently from 111027e to 1a29218 Compare November 28, 2022 15:13

[WIP] relates to C2-2197: setup the command serialization with tests

be5193f

ivansenic force-pushed the ise-c2-2197-dtos branch from 1a29218 to be5193f Compare November 28, 2022 15:19

ivansenic requested a review from amorton November 28, 2022 15:27

fix int test

4107157

ivansenic commented Nov 28, 2022

View reviewed changes

Ivan Senic added 4 commits November 28, 2022 17:06

update docker images on the ci failure

a84a1c4

add sha to the built image

a12a6f4

export correct image

f52a949

ignore validation test in the native image build

687831c

ivansenic commented Nov 28, 2022

View reviewed changes

tatu-at-datastax reviewed Nov 28, 2022

View reviewed changes

amorton reviewed Nov 29, 2022

View reviewed changes

review fixes

873aa89

ivansenic changed the title ~~[WIP] relates to C2-2197: setup the command serialization with tests~~ relates to C2-2197: setup the command serialization with tests Nov 29, 2022

add v3 to main class

2f9c15d

ivansenic mentioned this pull request Nov 30, 2022

relates to C2-2197: modeling command resolving and processing #10

Merged

3 tasks

tatu-at-datastax approved these changes Nov 30, 2022

View reviewed changes

ivansenic merged commit 65f05c2 into main Dec 1, 2022

ivansenic deleted the ise-c2-2197-dtos branch December 1, 2022 16:05

relates to C2-2197: setup the command serialization with tests #7

relates to C2-2197: setup the command serialization with tests #7

Conversation

ivansenic commented Nov 28, 2022 • edited Loading

ivansenic left a 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

Choose a reason for hiding this comment

tatu-at-datastax Nov 29, 2022 • 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

Choose a reason for hiding this comment

tatu-at-datastax Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic Nov 29, 2022 • 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

tatu-at-datastax Nov 29, 2022 • 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

amorton Nov 29, 2022 • 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

ivansenic commented Nov 29, 2022

tatu-at-datastax commented Nov 29, 2022

ivansenic commented Nov 30, 2022

tatu-at-datastax commented Nov 30, 2022

tatu-at-datastax left a comment

Choose a reason for hiding this comment

ivansenic commented Dec 1, 2022

ivansenic commented Nov 28, 2022 •

edited

Loading

tatu-at-datastax Nov 29, 2022 •

edited

Loading

tatu-at-datastax Nov 29, 2022 •

edited

Loading

ivansenic Nov 29, 2022 •

edited

Loading

tatu-at-datastax Nov 29, 2022 •

edited

Loading

amorton Nov 29, 2022 •

edited

Loading