Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DSP-428 Create res properties #235

Merged
merged 23 commits into from
Sep 23, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
823a044
feat(ontology): Init "create property"
kilchenmann Sep 7, 2020
c93db05
feat(ontology): Create property
kilchenmann Sep 9, 2020
5cb0fe1
test(ontology): Init test for res property
kilchenmann Sep 9, 2020
cc70297
feat(ontology): Create property
kilchenmann Sep 10, 2020
7375f2d
Merge branch 'master' into wip/dsp-428-res-properties
kilchenmann Sep 11, 2020
4cdf1f4
test(ontology): More test data
kilchenmann Sep 11, 2020
59de3c3
test(ontology): Add create property test
kilchenmann Sep 15, 2020
de21383
Merge branch 'master' into wip/dsp-428-res-properties
tobiasschweizer Sep 16, 2020
14d9515
test (ontologies endpoint): fix res prop creation test
Sep 16, 2020
f98e1e6
test (ontologies endpoint): use classes to create request object
Sep 16, 2020
01a9950
test (ontologies endpoint): add test for creating a link prop
Sep 16, 2020
0ef4b7d
feature (ontologies endpoint): Add cardinality for a property on a re…
Sep 17, 2020
f762399
fix (ontologies): fix typo
Sep 18, 2020
ca6ddb9
refactor (ontologies): make isInherited prop optional
Sep 18, 2020
65ef34a
feature (test app): add method to add cardinality
Sep 18, 2020
a419211
feature (test app): use unique name
Sep 18, 2020
b7f0bca
fix (index.ts): add missing export
Sep 18, 2020
6da6b77
Merge branch 'master' into wip/dsp-428-res-properties
tobiasschweizer Sep 22, 2020
2cdeb52
feature (test app): use testonto consistently
Sep 22, 2020
f0dc4ad
Merge branch 'master' into wip/dsp-428-res-properties
tobiasschweizer Sep 22, 2020
34dba9e
test (e2e): test property creation
Sep 22, 2020
b5e95da
test (e2e): test adding a cardinality
Sep 22, 2020
1f9087c
test (e2e): test property deletion
Sep 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# Reference: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839

# User-specific stuff
.vscode/**
.vscode
.idea/**/workspace.xml
.idea/**/tasks.xml
.idea/**/usage.statistics.xml
Expand Down
24 changes: 24 additions & 0 deletions scripts/v2-test-data-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,30 @@
"source": "/v2/ontologies/create-class-without-cardinalities-response.json",
"destination": "./test/data/api/v2/ontologies/create-class-without-cardinalities-response.json"
},
{
"source": "/v2/ontologies/create-value-property-request.json",
"destination": "./test/data/api/v2/ontologies/create-value-property-request.json"
},
{
"source": "/v2/ontologies/create-value-property-response.json",
"destination": "./test/data/api/v2/ontologies/create-value-property-response.json"
},
{
"source": "/v2/ontologies/create-link-property-request.json",
"destination": "./test/data/api/v2/ontologies/create-link-property-request.json"
},
{
"source": "/v2/ontologies/create-link-property-response.json",
"destination": "./test/data/api/v2/ontologies/create-link-property-response.json"
},
{
"source": "/v2/ontologies/add-cardinalities-to-class-nothing-request.json",
"destination": "./test/data/api/v2/ontologies/add-cardinalities-to-class-nothing-request.json"
},
{
"source": "/v2/ontologies/add-cardinalities-to-class-nothing-response.json",
"destination": "./test/data/api/v2/ontologies/add-cardinalities-to-class-nothing-response.json"
},
{
"source": "/v2/resources/testding.json",
"destination": "./test/data/api/v2/resources/testding.json"
Expand Down
120 changes: 110 additions & 10 deletions src/api/v2/ontology/ontologies-endpoint-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,24 @@ import { AjaxResponse } from "rxjs/ajax";
import { catchError, map, mergeMap } from "rxjs/operators";
import { ApiResponseError } from "../../../models/api-response-error";
import { Constants } from "../../../models/v2/Constants";
import { AddCardinalityToResourceClass } from "../../../models/v2/ontologies/create/add-cardinality-to-resource-class";
import { CreateOntology } from "../../../models/v2/ontologies/create/create-ontology";
import { CreateResourceClass } from "../../../models/v2/ontologies/create/create-resource-class";
import { CreateResourceClassPayload, NewResourceClass } from "../../../models/v2/ontologies/create/create-resource-class-payload";
import {
CreateResourceClassPayload,
NewResourceClass
} from "../../../models/v2/ontologies/create/create-resource-class-payload";
import { CreateResourceProperty } from "../../../models/v2/ontologies/create/create-resource-property";
import {
CreateResourcePropertyPayload,
NewResourcePropertyPayload
} from "../../../models/v2/ontologies/create/create-resource-property-payload";
import { DeleteOntologyResponse } from "../../../models/v2/ontologies/delete/delete-ontology-response";
import { OntologiesMetadata, OntologyMetadata } from "../../../models/v2/ontologies/ontology-metadata";
import { OntologyConversionUtil } from "../../../models/v2/ontologies/OntologyConversionUtil";
import { ReadOntology } from "../../../models/v2/ontologies/read/read-ontology";
import { ResourceClassDefinitionWithAllLanguages } from "../../../models/v2/ontologies/resource-class-definition";
import { ResourcePropertyDefinitionWithAllLanguages } from "../../../models/v2/ontologies/resource-property-definition";
import { UpdateOntology } from "../../../models/v2/ontologies/update-ontology";
import { Endpoint } from "../../endpoint";

Expand Down Expand Up @@ -66,11 +76,11 @@ export class OntologiesEndpointV2 extends Endpoint {
}

/**
* Requests metadata about all ontologies from a specific project
*
* @param projectIri the IRI of the project
* @return OntologiesMetadata or an error
*/
* Requests metadata about all ontologies from a specific project
*
* @param projectIri the IRI of the project
* @return OntologiesMetadata or an error
*/
getOntologiesByProjectIri(projectIri: string): Observable<OntologiesMetadata | ApiResponseError> {

return this.httpGet("/metadata/" + encodeURIComponent(projectIri)).pipe(
Expand All @@ -90,7 +100,7 @@ export class OntologiesEndpointV2 extends Endpoint {

/**
* Creates a new ontology.
*
*
* @param ontology The ontology to be created
*/
createOntology(ontology: CreateOntology): Observable<OntologyMetadata | ApiResponseError> {
Expand Down Expand Up @@ -136,7 +146,7 @@ export class OntologiesEndpointV2 extends Endpoint {

/**
* Create a resource class without cardinalities
*
*
* @param resClass The resource class to be created
*/
createResourceClass(resClass: CreateResourceClass): Observable<ResourceClassDefinitionWithAllLanguages | ApiResponseError> {
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we implement additional checks in this PR? I suggest to do it in a separate one..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newResClass.subClassOf = resClass.subClassOf;
newResClass.type = Constants.Class;

Expand All @@ -176,7 +186,7 @@ export class OntologiesEndpointV2 extends Endpoint {
/**
* Delete resource class
*
* @param updateOntology
* @param updateOntology with class IRI
*/
deleteResourceClass(updateOntology: UpdateOntology): Observable<OntologyMetadata | ApiResponseError> {

Expand All @@ -196,4 +206,94 @@ export class OntologiesEndpointV2 extends Endpoint {

}

/**
* Create a resource property
*
* @param resProp The resource property to be created
*/
createResourceProperty(resProp: CreateResourceProperty): Observable<ResourcePropertyDefinitionWithAllLanguages | ApiResponseError> {

const resPropPayload = new CreateResourcePropertyPayload();

// prepare ontology data for payload
resPropPayload.id = resProp.ontology.id;
resPropPayload.lastModificationDate = resProp.ontology.lastModificationDate;

// prepare new res class object for payload
const newResProperty = new NewResourcePropertyPayload();

newResProperty.id = resProp.ontology.id + Constants.Delimiter + resProp.name;

newResProperty.label = resProp.labels;
newResProperty.comment = (resProp.comments.length ? resProp.comments : resProp.labels);
newResProperty.subPropertyOf = resProp.subPropertyOf;
newResProperty.type = Constants.ObjectProperty;

newResProperty.subjectType = resProp.subjectType;
newResProperty.objectType = resProp.objectType;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (resProp.guiElement) {
newResProperty.guiElement = resProp.guiElement;
}
if (resProp.guiAttributes) {
newResProperty.guiAttributes = resProp.guiAttributes;
}

resPropPayload.resProperty = [newResProperty];

const payload = this.jsonConvert.serializeObject(resPropPayload);

return this.httpPost("/properties", payload).pipe(
mergeMap((ajaxResponse: AjaxResponse) => {
// TODO: @rosenth Adapt context object
// TODO: adapt getOntologyIriFromEntityIri
return jsonld.compact(ajaxResponse.response, {});
}), map((jsonldobj: object) => {
return OntologyConversionUtil.convertResourcePropertyResponse(jsonldobj, this.jsonConvert);
}),
catchError(error => {
return this.handleError(error);
})
);
}

/**
* Delete resource property
*
* @param updateOntology with property IRI
*/
deleteResourceProperty(updateOntology: UpdateOntology): Observable<OntologyMetadata | ApiResponseError> {

const path = "/properties/" + encodeURIComponent(updateOntology.id) + "?lastModificationDate=" + encodeURIComponent(updateOntology.lastModificationDate);

return this.httpDelete(path).pipe(
mergeMap((ajaxResponse: AjaxResponse) => {
// TODO: @rosenth Adapt context object
// TODO: adapt getOntologyIriFromEntityIri
return jsonld.compact(ajaxResponse.response, {});
}),
map(jsonldobj => {
return this.jsonConvert.deserializeObject(jsonldobj, OntologyMetadata);
}),
catchError(error => this.handleError(error))
);
}

addCardinalityToResourceClass(addCardinalityToResourceClass: AddCardinalityToResourceClass): Observable<ResourceClassDefinitionWithAllLanguages | ApiResponseError> {

return this.httpPost("/cardinalities", this.jsonConvert.serializeObject(addCardinalityToResourceClass)).pipe(
mergeMap((ajaxResponse: AjaxResponse) => {
// TODO: @rosenth Adapt context object
// TODO: adapt getOntologyIriFromEntityIri
return jsonld.compact(ajaxResponse.response, {});
}), map((jsonldobj: object) => {
return OntologyConversionUtil.convertResourceClassResponse(jsonldobj, this.jsonConvert);
}),
catchError(error => {
return this.handleError(error);
})
);

}

}
Loading