Prepare for ADE-1: rename tags

[alamers]

I was doing some testing from a Java perspective: downloading a generated Java client from Swagger UI (Swagger codegen) based on the example url scheme. (I think this would be the most common use case, either generating it from this repo, or from an implementing party directly, as this also generates the transport client.)

I noticed that the code generated is making use of the tags (now: release-candidate-milk, ...).
This then leads to API classes like in the enclosed image: "ReleaseCandidateMilkApi". Thus, the naming of these tags is important, not just for documentation. I suggest to follow the same version rules as for the url scheme and use "ade1-milk", "ade1-registration", ...

[ahokkonen]

I agree with that! Btw, have you had any issue with generating Animal/Location -identifiers into code?

[alamers]

Got interrupted in checking the generated model. Will try to do so later today :)

[alamers]

I am seeing some other issues with the default swagger codegen using the example url scheme. (I am assuming that most people will use swagger codegen so that's what I am focusing on now.)

Jotting down some notes for discussion:

I fixed a few missing relative paths (this seems broken in codegen, as it used the url scheme as a base, not the file it is currently reading):

• icarBreedingIdentifierType, icarConsignmentType, icarLocationIdentifierType, icarTraitLabelIdentifierType all used "$ref": "icarIdentifierType.json" and i replaced that with "$ref": "../types/icarIdentifierType.json"

Feels like a harmless compensation for a bug somewhere else?

Lets look at GET /../animals first:

• The url scheme currently defines an array of #/components/animal while that is actually a "$ref": "../collections/icarAnimalCoreCollection.json". So, an array of arrays.

• In the java code gen, this ultimately leads to a class Animal extends IcarResourceCollection which holds a List<IcarAnimalCoreResource>.

From a coding point of view, these names are clumsy. Since we have divided the schema's into folders, we could create a bit cleaner naming.

Wrt the Animal identifier:

• I (locally) changed icarAnimalIdentifierType to use allOf icarIdentifierType. This indeed gives a nicer result:

public class IcarAnimalCoreResource extends IcarResource {
  private IcarAnimalIdentifierType identifier = null;
  private List<IcarAnimalIdentifierType> alternativeIdentifiers = null;
  ...
}

and

public class IcarAnimalIdentifierType extends IcarIdentifierType {
...
}

and

public class IcarIdentifierType {
  private String id = null;
  private String scheme = null;
  ...
}

Suggestions:

1. in the example url scheme, remove the superfluous array
2. in the example url scheme, refer to #/components/animalCollection so we get an AnimalCollection class
3. rename the icarAnimalCoreResource.json to Animal.json (not sure, may introduce name clashes with business code. We could loose the CoreResource part though...)
4. always use full relative paths, also if referring to a file in same directory. (assume it is referenced from url-scheme directory)

[ahokkonen]

1 and 2. I can do both same time I'm doing identifier changes for animal/location/breed/trait

3. This is probably should be common decision, I'm not a big fan of using prefixes/suffixes in general, but I guess such naming is part of design decision.

4. I don't see this as a problem to use all relative references. Another solution would be to create URL Schema bundle on each merge (using actions), which will contain pre-generated JSON schema with all references and which will be used for Swagger Codegen. swagger-ui-watcher is on of the tool which can be used for that purpose.

[ahokkonen]

I made a pull request containing fixes for most of the things in that thread.

Code generation into CSharp using "openapi-generator" tools gives good result (also inheritance works well).
Some components generates with duplicates, but seems like they stay "not connected" into other resource dependencies and do not provide problems.

@alamers @cookeac @erwinspeybroeck

[ahokkonen]

@alamers as now all changes are merged into master you can try to generate Java code again to see if it work better from scratch? :)

On Windows system I did it with "openapi-geneator" (installed via NPM) calling (both for server side code):

.NET Core 3.1: openapi-geneator generate -g aspnetcore -i url-schemes/exampleUrlScheme.json -o output

Java: openapi-geneator generate -g java -i url-schemes/exampleUrlScheme.json -o output

There are several things to improve, but in general looks good to me

[alamers]

I just ran it from swagger again (via maven:

<groupId>io.swagger.codegen.v3</groupId>
<artifactId>swagger-codegen-maven-plugin</artifactId>
<version>3.0.18</version>

This still has some issues, so doesn't work 'out of the box'.
First: the IcarAnimalCoreResource looks good now (aside from the naming discussion) so that is excellent:

public class IcarAnimalCoreResource extends IcarResource {
  private IcarAnimalIdentifierType identifier = null;
  private List<IcarAnimalIdentifierType> alternativeIdentifiers = null;
  private IcarAnimalSpecieType specie = null;
  private IcarAnimalGenderType gender = null;
  private OffsetDateTime birthDate = null;
 ...
}

Swagger codegen trips over the duplicate declaration of member in both icarResourceCollection.json and the derived collections:icarAnimalCoreCollection.json. This leads to type erasure problems with that list and apparently is not handled well by Swagger codegen. It seems that we are not using the 'discriminator' tag here correctly. I haven't been able to fix it to a working version yet though. As far as I understand it, the discriminator should allow tools like codegen to create a strongly typed object. But it doesn't seem to be working for a collection as we have here, maybe only for inheritance.

If I remove 'member' and the discriminator 'memberType' from icarResourceCollection it generates fine. (aside from two comparable issues with discriminators in icarIdentifierType and icarEventCoreResource.json.

[alamers]

I also tried it with the openapi-generator as per your suggestion. As it is a fork of swagger codegen, it seems a bit more up-to-date and indeed produces better results. So maybe ignore Swagger codegen altogether?

Some notes:

• the type erasure / allOf / collection issue is still there, so I guess we're doing it wrong somewhere?
• it produces IcarAnimalCoreCollectionAllOf classes (and equivalents), no idea where those are coming from?

[ahokkonen]

@alamers seems like different tools gives different results which is not very good I suppose.

I've added discriminator to get codegen apply proper inheritance from superclasses (icarEventCoreResource, IdentifierType, etc.). Without discriminator all inherited classes generates as independent having internal declarations for all properties - this is at least with generating using "openapi-generator".

AllOf issue is bug/feature described here OpenAPITools/openapi-generator#3100. Main reason for this is having inline schemas with AllOf declaration.

Update:
Having discriminators as it is now works almost perfect with .NET generation, but seems like with Java it is an issue. In icarResourceCollection "member" -property probably can be removed if it is affecting errors in Java (in .NET it is just redefined/overwritten by derived classes without problem), but without discriminator I can't get inheritance to work in .NET... Played a lot with different generators/platforms and indeed they works differently in some cases. I'm not sure if we can to get code generation to work exactly same way on all platforms.

[alamers]

Just checked the Java version and the generation looks ok to me. The Collection classes have the 'member' field which is just a list again:

public class IcarDeviceCollection extends IcarResourceCollection {
  private List<IcarDeviceResource> member = null;
  ...
}

But what I understand from the discussions in the workgroup, this (superfluous) field is due to adopting the JSON-LD Hydra syntax? Anyway, it is not much of an obstacle.

[alamers]

So I'd propose to close this ticket, generation works fine and the original ticket has been solved:

[ahokkonen]

Well, personally I would'n use any of those resource collection in my final code :)
Instead I would prefer having some sort of generic solution like:

public class IcarResourceCollection<T> where T : IcarEventCoreResource {
    // yes, in C# we can use auto-implemented properties :)
    private List<T> Member {get; set; }
}

and use it for all event resource collections on API layer. But this is something you need to implement your own generator for.

I'm also agree that original ticket is solved here.

[alamers]

Yes, I suppose the collection classes won't be used much, just the endpoints to collect resources, not the collections themselves.

(And Java at least get's its capitalization correct... ;) )