Added TestExport to avoid creating unstructured json, fixes #860

[stalep]

Importing new test should work now correctly. Currently Experiments&/Comparisons are not correctly imported.

fixes #860

[johnaohara]

Some comments;

1. Please can we have some user docs for test export/import?

2. One thought, how do we handle different versions of test import/export? Or how would we in the future? One significant impact of this change is that we are bound to one version of an exported test for the version of Horreum running, so say you could not export a test from Horreum 0.11.1 and import into 0.13.4 if the test data structure changes between releases.

The old API would allow users to upload the json payload an Horreum had the ability to perform some form of check, but with this change we would return a HTTP 400 bad request if the JSON did not deserialise to a valid TestExport.

I think we should consider backwards compatibility between Horreum Versions. I am not saying we should make import/export backwards compatible now, but leave the possibility of supporting it in the future

3. We need to consider datastores? The default datastore is fine, but what about a Test with an Elastic backend etc? We either need to throw an error, or tell the user to configure a datastore first, or serialize datastore with the test. Serializing the datastore would require careful consideration about how to handle API tokens

[johnaohara]

Currently Experiments&/Comparisons are not correctly imported.

Please can you open an issue to track this

[stalep]

Some comments;

1. Please can we have some user docs for test export/import?

Yes!

2. One thought, how do we handle different versions of test import/export? Or how would we in the future?  One significant impact of this change is that we are bound to one version of an exported test for the version of Horreum running, so say you could not export a test from Horreum 0.11.1 and import into 0.13.4 if the test data structure changes between releases.

The old API would allow users to upload the json payload an Horreum had the ability to perform some form of check, but with this change we would return a HTTP 400 bad request if the JSON did not deserialise to a valid TestExport.

In reality there is no difference, before this change if someone would send a json that did not deserialize to a Test + the additional fields it would fail. With this change the JSON is a bit different, but it is typed and we avoid custom (de)serialization as it's handled by Quarkus.

I think we should consider backwards compatibility between Horreum Versions. I am not saying we should make import/export backwards compatible now, but leave the possibility of supporting it in the future

With using TestExport we can add additional fields later on and it would still work with older version.

3. We need to consider datastores?  The default datastore is fine, but what about a Test with an Elastic backend etc?   We either need to throw an error, or tell the user to configure a datastore first, or serialize datastore with the test.  Serializing the datastore would require careful consideration about how to handle API tokens

I have not considered datastores at all tbh, we could also add datastores to TestExport...?

[johnaohara]

In reality there is no difference, before this change if someone would send a json that did not deserialize to a Test + the additional fields it would fail. With this change the JSON is a bit different, but it is typed and we avoid custom (de)serialization as it's handled by Quarkus.

Under the hood Quarkus is using ObjectMapper to serialize/deserialize to/from json, we can still do the same without the custom deserialization. We can still enforce the type on the API, accepting different versions of concrete types.

If we don't we are locking an instance of Horreum to only accept Tests exported from another Horreum instance that has exported the same TestExport entity.

Adding fields might be fine, but if we remove required fields, change their type, or change the relationship between the test and other entities then it will break.

At the moment, we are effectively writing the DB structure to JSON. I don't see that staying stable between Horreum versions, and a change to the API to something like this, would still provide type safety, but give us the opportunity to evolve the entity graph contained in TestExport, whilst also supporting backward compatibility;

    @NotNull
    @JsonProperty( required = true )
    @Schema(type = SchemaType.OBJECT,
            oneOf = {
                TestExport_v1.class,
                    TestExport_v2.class
    })
    public ObjectNode export(@PathParam("id") int testId);

[johnaohara]

I have not considered datastores at all tbh, we could also add datastores to TestExport...?

We could, but how do we handle the secrets? This relates to secrets in the existing export as well. We can't encrypt/decrypt between Horreum instances that have different horreum.db.secret properties.

We might not want to handle that problem in this PR, so we could just add Datastore to the output and open another issue to handle secrets in the exported TestExport definition

[stalep]

If we don't we are locking an instance of Horreum to only accept Tests exported from another Horreum instance that has exported the same TestExport entity.

I think that's a fair limitation actually :)

Adding fields might be fine, but if we remove required fields, change their type, or change the relationship between the test and other entities then it will break.

Yes, and as I answered above it's a fair limitation imo, but I see your point.

At the moment, we are effectively writing the DB structure to JSON. I don't see that staying stable between Horreum versions, and a change to the API to something like this, would still provide type safety, but give us the opportunity to evolve the entity graph contained in TestExport, whilst also supporting backward compatibility;

Yes, I also see that changing in the not too distant future. What we then will end up with is a lot of parsing logic to support "old" versions of Horreum none might use...
I need to think about this..

[stalep]

We might not want to handle that problem in this PR, so we could just add Datastore to the output and open another issue to handle secrets in the exported TestExport definition

Yeah, that's a good suggestion.

[johnaohara]

If we don't we are locking an instance of Horreum to only accept Tests exported from another Horreum instance that has exported the same TestExport entity.

I think that's a fair limitation actually :)

Adding fields might be fine, but if we remove required fields, change their type, or change the relationship between the test and other entities then it will break.

Yes, and as I answered above it's a fair limitation imo, but I see your point.

Ok, let us flip it round another way.. We have so far said that the REST API needs to be stable between major versions. What we are now saying is that the REST API AND the DB schema needs to stay stable for a major version, if we make a change to the TestExport entity graph, we need to cut a new "major" version.

[johnaohara]

Yes, I also see that changing in the not too distant future. What we then will end up with is a lot of parsing logic to support "old" versions of Horreum none might use...
I need to think about this..

Parsing logic does not need to be complicated, it could be something like:

    public void importTest(ObjectNode newTest){
        if ( !newTest.has("version"))
            throw new BadRequestException("Missing field 'version' from the test definition");
        
        String version = newTest.get("version").asText();
        
        switch (version):
            case "1.0":
                TestExport_v1_0 test = mapper.treeToValue(newTest, TestExport_v1.class);
                break;
            case "1.1":
                TestExport_v1_1 test = mapper.treeToValue(newTest, TestExport_v2.class);
                break;
            default:
                throw new BadRequestException("Unknown version: " + version);
        }

The might be custom Mapping code, but we could have a base abstract Mapping for a major version, and the subclasses could override only where there has been minor changes.

[johnaohara]

@stalep I am not suggesting that we implement versioning now, but right now what we can do is change;

void importTest(TestExport testExport);

to

void importTest(ObjectNode testExport);

    TestExport testExportObj = mapper.treeToValue(testExport, TestExport.class);

With the API configured as;

   @GET
   @Path("{id}/export")
   @APIResponseSchema(value = TestExport.class,
           responseDescription = "A Test defintion formatted as json",
           responseCode = "200")
   ObjectNode export(@PathParam("id") int testId);

...

   @POST
   @Path("import")
   @APIResponse(responseCode = "204", description = "Import a new test")
   @RequestBody(content = @Content( mediaType = MediaType.APPLICATION_JSON,
           schema = @Schema( type = SchemaType.STRING, implementation = TestExport.class)) )
   @Operation(description="Import a previously exported Test")
   void importTest(ObjectNode testExport);

This would not lock the API into a particular version and we get all the benefits of type safety and auto parsing

[stalep]

Currently Experiments&/Comparisons are not correctly imported.

Please can you open an issue to track this

Ah, you might have misunderstood. Now in main, experiments&comparisons are not working correctly. #860 is tracking that. With this pr it all works (I think).

[johnaohara]

Currently Experiments&/Comparisons are not correctly imported.

Please can you open an issue to track this

Ah, you might have misunderstood. Now in main, experiments&comparisons are not working correctly. #860 is tracking that. With this pr it all works (I think).

Ah, sorry, I interpreted Currently Experiments&/Comparisons are not correctly imported. as it was not working with this PR so was asking to open an issue so we could fix that. but if this PR fixes that problem, then please ignore :)

[stalep]

@stalep I am not suggesting that we implement versioning now, but right now what we can do is change;

void importTest(TestExport testExport);

to

void importTest(ObjectNode testExport);

    TestExport testExportObj = mapper.treeToValue(testExport, TestExport.class);

With the API configured as;

   @GET
   @Path("{id}/export")
   @APIResponseSchema(value = TestExport.class,
           responseDescription = "A Test defintion formatted as json",
           responseCode = "200")
   ObjectNode export(@PathParam("id") int testId);

...

   @POST
   @Path("import")
   @APIResponse(responseCode = "204", description = "Import a new test")
   @RequestBody(content = @Content( mediaType = MediaType.APPLICATION_JSON,
           schema = @Schema( type = SchemaType.STRING, implementation = TestExport.class)) )
   @Operation(description="Import a previously exported Test")
   void importTest(ObjectNode testExport);

This would not lock the API into a particular version and we get all the benefits of type safety and auto parsing

Ok, you convinced me :)
Just one last thing, what's the benefit of returning ObjectNode vs TestExport. I see the benefit of importing ObjectNode though.

[johnaohara]

Just one last thing, what's the benefit of returning ObjectNode vs TestExport. I see the benefit of importing ObjectNode though.

Good point, I think returning TestExport would be better.

[johnaohara]

Code looks great, I think just some things to clean up with the docs and the openAPI spec, and ofc package-lock.json :(

[johnaohara]

LGTM

[johnaohara]

Merged. Thanks a lot for your contribution