Fix editing schema

[MarekSuchanek]

closes #660

to be squash-merged

[dennisvang]

Hi @MarekSuchanek , this looks good too.
However, it would be very helpful if you could clarify some choices.
See review comments above&below.

[dennisvang]

Would it make sense to use schema.toBuilder() instead of MetadataSchemaVersion.builder()?
Or is there a specific reason not to?

For example:

        return schema.toBuilder()
                .published(false)  // assuming this needs to be false for a draft
                .state(MetadataSchemaState.DRAFT)
                .type(MetadataSchemaType.CUSTOM)
                .createdAt(Instant.now())  // not sure if these should be new timestamps or copies
                .updatedAt(Instant.now())
                .build();

[dennisvang]

Actually the same question applies to many of the other methods in MetadataSchemaMapper.
If most of the properties of an input argument are retained for a DTO method, I think it would make sense to signal this intent by the choice of builder method.

[MarekSuchanek]

That is true... should it be done as part of this PR?

[dennisvang]

should it be done as part of this PR?

Yes, I think it would make sense to include this change to the toDraft() method as part of this PR.

It fixes the issue and makes the method more consistent with similar methods like fromReleaseDTO and fromRemoteVersion.

[dennisvang]

@MarekSuchanek should we add a test, as in temp/test-for-issue-660?

[MarekSuchanek]

temp/test-for-issue-660

Simply copy-paste the test? Don't you want to merge that to this branch then?

[dennisvang]

temp/test-for-issue-660

Simply copy-paste the test? Don't you want to merge that to this branch then?

@MarekSuchanek Yes, the idea was to "merge" the test into this branch first.

Accidentally approved when I wanted to request changes as discussed in
#672 (comment)

[dennisvang]

Hi @MarekSuchanek,

If you agree, could you

• implement and/or check the suggested change for toDraftDTO (Fix editing schema #672 (comment))
• check if my test makes any sense?

Thanks in advance. :)

[MarekSuchanek]

Test looks OK, basic test to check the change.

As for toDraft - I did the suggested change, although when checking this I am not sure if consistency here makes sense. Semantically, it is more like creating a new draft from existing version and thus copying what shall be copied to the draft rather then transforming existing object to "new" one (now there must be more things to "reset"). Another option would be to rename the method, but I guess I do not really mind any way... so up to you 🙂

[dennisvang]

Thanks @MarekSuchanek , that's sounds reasonable. I'll look into it a bit more..

Do you think published should also be reset to false?

And what should happen with targetClasses?

(Actually I'm not even sure what those are... Probably SHACL targetClasses?)

[dennisvang]

Just for reference (from lombok docs):

[...] toBuilder(); it creates a new builder that starts out with all the values of this instance.

and

You can use @Builder for copy constructors: foo.toBuilder().build() makes a shallow clone.

[dennisvang]

Also for reference: The input and output types of toDraft and many other mapper methods were changed in 4318432 (as part of #519).

Here's the diff from v1.17.2 to develop for toDraft:

-    public MetadataSchemaDraft toDraft(MetadataSchema schema) {
-        return MetadataSchemaDraft.builder()
+    public MetadataSchemaVersion toDraft(MetadataSchemaVersion schema) {
+        return MetadataSchemaVersion.builder()
                 .uuid(schema.getUuid())
                 .name(schema.getName())
-                .name(schema.getName())
                 .description(schema.getDescription())
                 .abstractSchema(schema.isAbstractSchema())
                 .definition(schema.getDefinition())
-                .extendSchemas(schema.getExtendSchemas())
                 .suggestedResourceName(schema.getSuggestedResourceName())
                 .suggestedUrlPrefix(schema.getSuggestedUrlPrefix())
                 .build();
     }

In v1.17.2 a MetadataSchemaDraft was created from a MetadataSchema, whereas now (develop) a MetadataSchemaVersion is created from another MetadataSchemaVersion. The MetadataSchemaDraft entity has been removed, and the MetadataSchema entity has changed completely:

Click to see detailed diff

diff --git a/src/main/java/nl/dtls/fairdatapoint/entity/schema/MetadataSchema.java b/src/main/java/org/fairdatapoint/entity/schema/MetadataSchema.java
index f1c40354..997232b3 100644
--- a/src/main/java/nl/dtls/fairdatapoint/entity/schema/MetadataSchema.java
+++ b/src/main/java/org/fairdatapoint/entity/schema/MetadataSchema.java
@@ -1,6 +1,6 @@
 /**
  * The MIT License
- * Copyright © 2017 DTL
+ * Copyright © 2016-2024 FAIR Data Team
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -20,128 +20,39 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-package nl.dtls.fairdatapoint.entity.schema;
+package org.fairdatapoint.entity.schema;
 
-import jakarta.validation.constraints.NotBlank;
-import jakarta.validation.constraints.NotNull;
-import lombok.*;
-import org.bson.types.ObjectId;
-import org.springframework.data.annotation.Id;
-import org.springframework.data.annotation.PersistenceConstructor;
-import org.springframework.data.annotation.Transient;
-import org.springframework.data.mongodb.core.index.Indexed;
-import org.springframework.data.mongodb.core.mapping.Document;
+import jakarta.persistence.*;
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import lombok.experimental.SuperBuilder;
+import org.fairdatapoint.entity.base.BaseEntityCustomUUID;
+import org.fairdatapoint.entity.resource.MetadataSchemaUsage;
 
-import java.time.Instant;
-import java.util.ArrayList;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 
-@Document
+@Entity(name = "MetadataSchema")
+@Table(name = "metadata_schema")
 @Getter
 @Setter
+@NoArgsConstructor
 @AllArgsConstructor
-@Builder(toBuilder = true)
-public class MetadataSchema {
-
-    @Id
-    private ObjectId id;
-
-    @Indexed
-    private String uuid;
-
-    @Indexed
-    private String versionUuid;
-
-    @Indexed
-    private String versionString;
-
-    @NotNull
-    @Transient
-    private SemVer version;
-
-    @NotNull
-    @NotBlank
-    private String name;
-
-    @NotNull
-    private String description;
-
-    @NotNull
-    private String definition;
-
-    @NotNull
-    private Set<String> targetClasses = new HashSet<>();
-
-    @NotNull
-    private List<String> extendSchemas = new ArrayList<>();
-
-    @NotNull
-    private MetadataSchemaType type;
-
-    private String origin;
-
-    private String importedFrom;
-
-    private boolean latest;
-
-    private boolean published;
-
-    private boolean abstractSchema;
-
-    private String suggestedResourceName;
-
-    private String suggestedUrlPrefix;
-
-    private Instant createdAt;
-
-    private String previousVersionUuid;
-
-    @PersistenceConstructor
-    public MetadataSchema(
-            ObjectId id, String uuid, String versionUuid, String versionString,
-            String name, String description, String definition, Set<String> targetClasses,
-            List<String> extendSchemas, MetadataSchemaType type, String origin,
-            String importedFrom, boolean latest, boolean published, boolean abstractSchema,
-            String suggestedResourceName, String suggestedUrlPrefix, Instant createdAt,
-            String previousVersionUuid
-    ) {
-        this.id = id;
-        this.uuid = uuid;
-        this.versionUuid = versionUuid;
-        this.versionString = versionString;
-        this.name = name;
-        this.description = description;
-        this.definition = definition;
-        this.targetClasses = targetClasses;
-        this.extendSchemas = extendSchemas;
-        this.type = type;
-        this.origin = origin;
-        this.importedFrom = importedFrom;
-        this.latest = latest;
-        this.published = published;
-        this.abstractSchema = abstractSchema;
-        this.suggestedResourceName = suggestedResourceName;
-        this.suggestedUrlPrefix = suggestedUrlPrefix;
-        this.createdAt = createdAt;
-        this.previousVersionUuid = previousVersionUuid;
-    }
-
-    public void setVersionString(String versionString) {
-        this.versionString = versionString;
-        this.version = new SemVer(versionString);
-    }
-
-    public void setVersion(SemVer version) {
-        this.version = version;
-        this.versionString = version.toString();
-    }
-
-    public SemVer getVersion() {
-        if (this.version == null) {
-            this.version = new SemVer(this.versionString);
-        }
-        return version;
-    }
+@SuperBuilder
+public class MetadataSchema extends BaseEntityCustomUUID {
+
+    @OneToMany(fetch = FetchType.LAZY, cascade = CascadeType.ALL,
+            orphanRemoval = true, mappedBy = "schema")
+    private List<MetadataSchemaVersion> versions;
+
+    @OrderBy("orderPriority")
+    @OneToMany(mappedBy = "extendedMetadataSchema", cascade = CascadeType.ALL,
+            orphanRemoval = true, fetch = FetchType.LAZY)
+    private List<MetadataSchemaExtension> extensions;
+
+    @OrderBy("orderPriority")
+    @OneToMany(mappedBy = "usedMetadataSchema", cascade = CascadeType.ALL,
+            orphanRemoval = true, fetch = FetchType.LAZY)
+    private List<MetadataSchemaUsage> usages;
 }

[dennisvang]

Also for reference: Here's a simplified overview of changes to some of the schema entities from v1.17.2 to current develop, showing the data type of fields present in each entity.

field	v1.17.2 MetadataSchema	v1.17.2 MetadataSchemaDraft	v2.0.0-develop MetadataSchema	v2.0.0-develop MetadataSchemaVersion
abstractSchema	boolean	boolean		boolean
createdAt	Instant	Instant	Instant	Instant
definition	String	String		String
description	String	String		String
extendSchemas	List	List	(extensions?)
importedFrom	String			String
latest	boolean
name	String	String		String
origin	String			String
previousVersion				MetadataSchemaVersion
previousVersionUuid	String
published	boolean			boolean
schema				MetadataSchema
state				MetadataSchemaState
suggestedResourceName	String	String		String
suggestedUrlPrefix	String	String		String
targetClasses	Set	Set		List
type	MetadataSchemaType			MetadataSchemaType
updatedAt		Instant	Instant	Instant
uuid	String	String	UUID	UUID
version	SemVer			String
versionString	String
versionUuid	String

This shows that many fields have been moved, some have been removed, and some have been added. In addition, some field data types have changed.

Note that reverse relations are excluded from the table (i.e. relational fields that have mappedBy).

Note that, for example, fromChangeDTO and toDraftDTO have MetadataSchemaVersion draft arguments. However, there is nothing that ensures these are actually drafts, as in having .state(MetadataSchemaState.DRAFT). This was not an issue in v1.17.2, because that actually had a dedicated draft class: MetadataSchemaDraft draft.

[dennisvang]

And what should happen with targetClasses?

To answer my own question: These need not be touched.

The MetadataSchemaMapper.toDraft() method is only used in MetadataSchemaService.updateSchemaDraft(), which passes the new draft to MetadataSchemaMapper.fromChangeDTO():

FAIRDataPoint/src/main/java/org/fairdatapoint/service/schema/MetadataSchemaService.java

Line 141 in 8fb6443

metadataSchemaMapper.fromChangeDTO(reqDto, baseDraft)

fromChangeDTO() then rebuilds the list of target classes, based on the "new" schema definition:

FAIRDataPoint/src/main/java/org/fairdatapoint/service/schema/MetadataSchemaMapper.java

Lines 66 to 70 in 8fb6443

	.targetClasses(
	MetadataSchemaShaclUtils
	.extractTargetClasses(dto.getDefinition())
	.stream().toList()
	)

[dennisvang]

So what is published used for? It's not immediately clear from the source.