kit-data-manager / ro-crate-java

Java library to create and modify RO-Crates.
https://kit-data-manager.github.io/webpage/ro-crate-java/
Apache License 2.0
2 stars 3 forks source link

Default constructor creates invalid crate #35

Closed Code42Cate closed 2 years ago

Code42Cate commented 2 years ago

This is a really small issue, but if you create an empty RO Crate with the builder and then print the JSON metadata

RoCrate crate = new RoCrate.RoCrateBuilder().build();
System.out.println(crate.getJsonMetadata());

You get this:

{
   "@context":"https://w3id.org/ro/crate/1.1/context",
   "@graph":[
      {
         "@id":"./",
         "@type":"Dataset"
      },
      {
         "about":{
            "@id":"./"
         },
         "conformsTo":{
            "@id":"https://w3id.org/ro/crate/1.1"
         },
         "@id":"ro-crate-metadata.json",
         "@type":"CreativeWork"
      }
   ]
}

This seems to be the minium valid RO Crate.

But if you do the same with the RoCrate constructor:

RoCrate crate = new RoCrate();
System.out.println(crate.getJsonMetadata());

You get this:

{
   "@context":"https://w3id.org/ro/crate/1.1/context",
   "@graph":[
      null,
      null
   ]
}

Shouldn't they be the same?

Code42Cate commented 2 years ago

Adding

    rootDataEntity = new RootDataEntity.RootDataEntityBuilder()
        .build();
    jsonDescriptor = new ContextualEntity.ContextualEntityBuilder()
        .setId(ID)
        .addType("CreativeWork")
        .addIdProperty("about", "./")
        .addIdProperty("conformsTo", RO_SPEC)
        .build();

to the RoCrate constructor would fix this issue, then the RoCrate empty constructor would do the same as RoCrateBuilder empty constructor. Not sure if I like that solution though.

Pfeil commented 2 years ago

Hm. The solution you propose is fine, although it would be nice if we could reuse the logic of the builder somehow, as it is doing the work properly already.

What really makes me think: Do we need to be able to create a crate without the builder? Couldn't we make the constructor private? If we really would like to have some shortcut for a default crate, we could create a static method returning what the default of a simple builder is.

// long variant
RoCrate crate = new RoCrate.RoCrateBuilder().build();
// short / semantically simple variant
RoCrate crate = RoCrate.empty();
// or
RoCrate crate = RoCrate.builder().build();

Not sure if we can use a constructor somehow to reuse the builder logic in a way it makes sense.

Code42Cate commented 2 years ago

If we remove the ability to create crates without the builder, then we really need to add the functionality to put an existing crate into a builder (see this issue #36)

Pfeil commented 2 years ago

Indeed. I am not saying this is a fixed plan, but in general the builder pattern can give quiet some control (about how the API is being used), compared to setters. But here it is just about new crates and reusing the builder for that, isn't it? An implementation could be as simple as

// make constructors private, and then add:
public static RoCrate empty() {
    return new RoCrateBuilder().build();
}

or "simply" use the builder.

Pfeil commented 2 years ago

After some thinking, I suggest I simply fix the constructor for 1.x.x and for 2.x.x will implement another solution with less duplication etc.

Code42Cate commented 2 years ago

Good. Do we want to create a project for tracking the planned 2.0 changes or is the 2.0 milestone in issues/prs good enough for now?

Pfeil commented 2 years ago

With #42 it will be fixed in the next release. For 2.0 we plan to make this and other places in the API cleaner and safer. But as the actual issue is being fixed, I will close this.