DSP-428 Create res properties #235
Conversation
kilchenmann
added
enhancement
flavens
added
enhancement
| import { IdConverter } from "../../custom-converters/id-converter"; | ||
| import { UpdateOntology } from "../update-ontology"; | ||
|
|
||
| @JsonObject("CreateResourceProperty") |
There was a problem hiding this comment.
@kilchenmann If I understand correctly, only the classes with he suffix "payload" are actually going to be sent to the knora-api. So CreateResourcePropertyPayload actually has to be serialised to JSON-LD, whereas CreateResourceProperty is expected as an argument by the method createResourceProperty and an instance of CreateResourcePropertyPayload is then created from it.
Why would we need to have TS decorators for json2typescript on CreateResourceProperty then? Since this is not going to be serialized directly, we could even add a constructor to make it easier for the user to create an instance of CreateResourceProperty .
There was a problem hiding this comment.
Yes of course, this would be the better way. Thanks.
There was a problem hiding this comment.
I will do this as part of https://dasch.myjetbrains.com/youtrack/issue/DSP-685
| @@ -151,7 +161,7 @@ export class OntologiesEndpointV2 extends Endpoint { | |||
| const newResClass = new NewResourceClass(); | |||
| newResClass.id = resClass.ontology.id + Constants.Delimiter + resClass.name; | |||
| newResClass.label = resClass.labels; | |||
| newResClass.comment = resClass.comments; | |||
| newResClass.comment = (resClass.comments.length ? resClass.comments : resClass.labels); | |||
There was a problem hiding this comment.
@kilchenmann Is this supposed to be a fallback if no comments are provided? But then how would we know that the labels array is not empty?
There was a problem hiding this comment.
Yes, this is a kind of fallback. The labels are a mandatory value, they shouldn't be empty. But I think we do not check that, do we?
There was a problem hiding this comment.
The problem is that with json2typescript, all properties have an initial value, so technically speaking, all props are set. But the initial value does not make any sense, e..g, an empty string.
So yes, there could be some checks in the endpoint's method for empty strings and empty arrays.
There was a problem hiding this comment.
Should we implement additional checks in this PR? I suggest to do it in a separate one..
There was a problem hiding this comment.
I will do it in https://dasch.myjetbrains.com/youtrack/issue/DSP-685, I added a note
|
@kilchenmann tomorrow I will complete the test for deleting an ontology. The DELETE request does not have a body, and the response will be the ontology's metadata (https://docs.knora.org/03-apis/api-v2/ontology-information/#deleting-a-property). |
|
@kilchenmann I think the test for property deletion is fine: 2cdeb52 |
|
@kilchenmann I will add the missing e2e tests and then this PR is ready for the final review. |
|
Sounds great, thanks! |
|
@kilchenmann I created https://dasch.myjetbrains.com/youtrack/issue/DSP-685. I will work on this once this PR is merged. Could please check if this PR implements what is required for https://dasch.myjetbrains.com/youtrack/issue/DSP-428? You won't be to review this PR I think since you initially created it. |
| newResProperty.type = Constants.ObjectProperty; | ||
|
|
||
| newResProperty.subjectType = resProp.subjectType; | ||
| newResProperty.objectType = resProp.objectType; |
There was a problem hiding this comment.
Can we simplify the CreateResourceProperty or at least make the objectType optional. If objectType is not defined we can define it from subPropertyOf.
// If the new property is a subproperty of knora-api:hasValue, its knora-api:objectType must be one of the built-in subclasses of knora-api:Value.
// If the new property is a subproperty of knora-base:hasLinkTo, its knora-api:objectType must be a subclass of knora-api:Resource
newResProperty.objectType = (resProp.subPropertyOf === Constants.HasValue ? Constants.Value : Constants.Resource)Not sure if all Constants are defined. And maybe this is not a good solution as the subPropertyOfcould be a subproperty of the mentioned knora-api:hasValue and knora-api:hasLinkTo.
Or what do you think @tobiasschweizer?
There was a problem hiding this comment.
I think we could do some consistency checks for value and link props. Or we could make specific classes for value and link props. Would that help?
There was a problem hiding this comment.
A user of the method needs a deep knowledge of the ontology stuff. I thought that the object you hand over to the method could be simplified. At this point it is similar to the payload. And you have to take care that you pass the correct object-type matching the subPropertyOf.
I had this idea already in mind when I created the method and the CreateResourceProperty class and thought to clean it up at the end.
I'll be able to use it in DSP-App. But if somebody else uses it, it gets complicated very fast. For the moment it's ok for me. It was just an idea / suggestion.
There was a problem hiding this comment.
It's definitely worth thinking about. I will keep that in kind when doing the refactoring in https://dasch.myjetbrains.com/youtrack/issue/DSP-685
There was a problem hiding this comment.
I made a note in https://dasch.myjetbrains.com/youtrack/issue/DSP-685
|
@kilchenmann Do you think this PR can be merged so I can start the refactoring? |
Yes I think so. Let's merge it |
Resolves DSP-428