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

Rename properties and support for name variants #67

Merged
merged 5 commits into from
Aug 12, 2019

Conversation

benbosman
Copy link
Member

This PR contains two minor changes to the REST contract:

  • The leftLabel is renamed to leftWardLabel at the relationship type level to clarify e.g. isAuthorOfPublication is the name of the arrow pointing from the right item to the left item. The term leftLabel created some confusion in the past.
  • Support for name variants has also been included with an leftWardLabel property at the relationship level to specify an alternative name used in the arrow pointing from the right item to the left item.

The name variants will be exposed in the virtual metadata as well, so this direct property will primarily be used for storing and updating the label.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@benbosman : Overall this looks good, but I did find a few things that I think need improvement. None of them seem major though.

relationships.md Outdated Show resolved Hide resolved
relationships.md Outdated Show resolved Hide resolved
relationships.md Outdated
@@ -104,6 +106,11 @@ A sample CURL command would be:
curl -i -X POST 'https://dspace7-entities.atmire.com/rest/api/core/relationships?relationshipType=1' -H 'Authorization: Bearer eyJhbGciO…' -H "Content-Type:text/uri-list" --data 'https://dspace7-entities.atmire.com/rest/api/core/items/12623672-25a9-4df2-ab36-699c4c240c7e \n https://dspace7-entities.atmire.com/rest/api/core/items/5a3f7c7a-d3df-419c-8a2-f00ede62c60a'
```

Including a name variant would result in:
```
curl -i -X POST 'https://dspace7-entities.atmire.com/rest/api/core/relationships?relationshipType=1&leftWardLabel=Name%20variant%201' -H 'Authorization: Bearer eyJhbGciO…' -H "Content-Type:text/uri-list" --data 'https://dspace7-entities.atmire.com/rest/api/core/items/12623672-25a9-4df2-ab36-699c4c240c7e \n https://dspace7-entities.atmire.com/rest/api/core/items/5a3f7c7a-d3df-419c-8a2-f00ede62c60a'
Copy link
Member

@tdonohue tdonohue Jul 23, 2019

Choose a reason for hiding this comment

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

I think we may want to improve this example or explain it further. I'm assuming what you mean is that name variants are always specified on labels? So, if your name variant is "Smith, Jane" then you'd assign the value of the appropriate label (either leftwardLabel or rightwardLabel) to be "Smith, Jane". Is that correct?

If that's correct, I think we need to be clearer in our contract about the fact that you are expected to use the appropriate label to assign a name variant (and that in this example, the name variant assigned is "Name variant 1"). Currently, this example is vague as it's unclear if "Name variant 1" is the name variant text itself...or if it's a reference/identifier to a separate name variant object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did want the REST contract to remain abstract about whether it would contain the name or a reference as this was still to be decided. For REST, it doesn't really matter which solution it is.
We've briefly touched this topic last meeting during the demo of the mockups, and can continue the discussion. To avoid any confusion, it does make sense to update the examples with what will eventually be used, but I assume it doesn't have to block starting the REST API implementation

Copy link
Member

Choose a reason for hiding this comment

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

@benbosman : I agree, this isn't a blocker, but the examples here should be updated with more realistic examples (once a decision is made). Otherwise the vagueness may make it difficult to validate the implementation against the REST contract.

All that said, I'm fine with this contract for now. I just think it will need an update once implementation decisions have been made.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me overall. As a sidenote, once implementation decisions are made regarding setting/updating name variants, I think the name variant example in this REST Contract should be updated. Currently, as noted above, the example is vague. Ideally, it should give a realistic example.

relationships.md Outdated

```json
{
"id": 530,
"relationshipTypeId": 0,
"leftPlace": 1,
"leftwardLabel": "Name variant 1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @benbosman!
As I understood, there will be both a leftwardLabel and rightwardLabel on relationship_type (renaming the existing ones) and here, on relationship.
Will this leftwardLabel or rightwardLabel override what is defined by the relationship_type? I'm confused regarding the chosen names. What will held what? And I think others will also get confused. Perhaps it's best to get a different property name for the relationship object like a relation attribute (something like leftRelationAttribute or rightRelationAttribute).
The example shows "relationshipTypeId": 0 but there isn't any particular reason to be 0, right? The relation type it's always mandatory, even with "labels".

Copy link
Member Author

Choose a reason for hiding this comment

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

@paulo-graca

  • The leftwardLabel on the relationship type is e.g. isAuthorOfPublication, it is the label used to describe the type of relationship
  • The leftwardLabel on the relationship is e.g. "Donald, Mike", it is the label used to describe the left item (or the name variant)

I didn't realize there was still a relationshipTypeId in the relationship implementation. This must be a left-over from when it was not a HAL link yet. This will need to be removed from the implementation (it's not used or populated anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thank you @benbosman for enlighten me! But it's a "label" (at the relationship level) or it's a relationship "attribute" or "property"? This might influence the naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

@paulo-graca it's a property of the relationship.
The reason why I used the term label is because it labels the left or right item from the point of view of the other item.
Would it be clearer to use leftwardNameVariant and rightwardNameVariant. Or would this make too many assumptions and make it less usable when used for non-person objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you agree with this suggestion?
Replacing of relationship.leftwardLabel with relationship.leftwardProperty and relationship.rightwardLabel with relationship.rightwardProperty and leave relationship_type.leftwardLabel and relationship_type.rightwardLabel as was suggested. My point of view is that those relationship values will have a different purpose than what is used on the relationship_type.

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual name doesn't matter too much for me, I'm probably a bit too closely involved
In DSpace 7 it would be used for name variants only (but it supports name variants for e.g. an Org Unit as well)
In DSpace 8 it may be expanded to support metadata completely

@tdonohue what name would you consider most suitable since you have already reviewed this PR?

Copy link
Member

@tdonohue tdonohue Aug 9, 2019

Choose a reason for hiding this comment

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

As I noted in Slack, I understand @paulo-graca's point here. The word "label" is seemingly being used for two different purposes. In the case of "relationship_type" the label describes the type of the relationship (e.g. "isAuthorOfPublication"). In the case of "relationship" the label is almost like metadata (e.g. "Smith, Jane"), or could be considered the "value" of the type "isAuthorOfPublication" (as essentially we are saying here "isAuthorOfPublication = Smith, Jane").

I admit, I don't feel strongly either way (as this could be described in documentation to clarify). But, if we were to go this route, I wonder if we should drop the word "label" altogether and replace it with better terms.

For example:

  • relationship_type.leftwardType = "isAuthorOfPublication" (This is the Type of the Relationship)
  • relationship.leftwardValue = "Smith, Jane" (This is the value of the Type of the Relationship)

leftwardValue could also be called leftwardMetadata (but metadata might be too generic of a term too).

I don't know if that helps this discussion, but it might help clarify the double meanings of "label", without requiring a lot of extra documentation.

Copy link
Contributor

@paulo-graca paulo-graca Aug 9, 2019

Choose a reason for hiding this comment

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

That's less confusing to me!

relationships.md Outdated
"leftLabel": "isAuthorOfPublication",
"rightLabel": "isPublicationOfAuthor",
"leftwardLabel": "isAuthorOfPublication",
"rightwardLabel": "isPublicationOfAuthor",
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid any further confusion and to clarify others, I think we could swap, or reorder to:

"rightwardLabel": "isPublicationOfAuthor",
"leftwardLabel": "isAuthorOfPublication",

Starting firstly with the rightward. In my mind, the relations always starts from the left item (Publication) to right item (Author) and this is expressed by the rightwardLabel . If in all produced documentation, regarding relations, the Atmire's picture and this order is showed, I think others will follow along easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

@paulo-graca would that be the only order you would want to be updated?
@tdonohue would you agree this helps make it easier to understand?

Copy link
Member

Choose a reason for hiding this comment

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

@benbosman : I don't feel strongly enough either way here. I'm fine with it being reordered in the documentation. But, in the actual response it might appear in any order (as I don't think order matters there at all)

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

The proposed changes make sense, indeed this is a further example that imho show how the metadatavalue/authority framework approach where the textvalue is used for local variation would been a better starting point that rebuild from scratch...
anyway, that said I'm fine with the PR +1

@benbosman benbosman merged commit 90f29aa into DSpace:master Aug 12, 2019
@benbosman benbosman deleted the renameLeftRightRelationships branch March 6, 2020 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants