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

feat(api-v2): Specify custom IRIs when creating resources/values #1646

Merged
merged 22 commits into from Jun 10, 2020

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented May 13, 2020

User story: https://dasch.myjetbrains.com/youtrack/issue/DSP-159

  • Have CreateResourceRequestV2.fromJsonLD accept:
    • a custom resource IRI
    • custom value IRIs
    • custom value UUIDs
  • Have CreateValueRequestV2.fromJsonLD accept:
    • a custom value IRI
    • a custom value UUID
    • a custom value creation date
  • Have ResourcesResponderV2 and ValuesResponderV2 use this information.
  • Add tests.
  • Update docs.

@benjamingeer benjamingeer self-assigned this May 13, 2020
@SepidehAlassi
Copy link
Contributor

@benjamingeer looking at the code I see that: when a resource with multiple values is being created, the creationDate of the resource is also assigned to its values. Since a resource can be created with custom creationData, I believe that is enough for the checksum. Then the values will have the correct creationDate anyway. What do you think?

In my opinion, custom creationDate should only be used for creating single values, i.e. CreateValueRequestV2 should accept a custom creationDate for a value.

@benjamingeer
Copy link
Author

Yes, good analysis!

@SepidehAlassi
Copy link
Contributor

The following refactoring step

  • Refactor resource creation so that generating random resource IRIs, and checking new resource IRIs for uniqueness, happens in ResourcesResponderV2 while holding a lock on the new IRI.

will happen in DSP-355

@SepidehAlassi
Copy link
Contributor

@benjamingeer eventually the tests passed!

Copy link
Author

@benjamingeer benjamingeer left a comment

Choose a reason for hiding this comment

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

After thinking about this some more, I think we actually need to accept a custom value creation date for each value even when creating a new resource. Imagine this scenario:

  1. On a test server, someone creates a new resource
  2. Then they add a value.
  3. Maybe they update the value several times.
  4. They cite the ARK URL for one of the updated versions of the value.
  5. They then transfer the whole project to the live server.

The ARK URL should still work, which means that the updated value needs to have its own value creation date, not the value creation date of the resource.

Sorry I didn't think this through well enough before.

@SepidehAlassi
Copy link
Contributor

After thinking about this some more, I think we actually need to accept a custom value creation date for each value even when creating a new resource.

In that case, if no custom creation date is provided for the value of a new resource, the creation date of the resource should be assigned to the value, right?

@SepidehAlassi
Copy link
Contributor

SepidehAlassi commented Jun 9, 2020

@benjamingeer last commit contains the implementation of "create resources with values that have custom creation dates". Can you please tell me your opinion?

Copy link
Author

@benjamingeer benjamingeer left a comment

Choose a reason for hiding this comment

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

last commit contains the implementation of "create resources with values that have custom creation dates". Can you please tell me your opinion?

Looks good to me. I'm just talking to Ivan about some questions I have... I think we may need more than custom IRIs to make this whole thing work...

@benjamingeer
Copy link
Author

In that case, if no custom creation date is provided for the value of a new resource, the creation date of the resource should be assigned to the value, right?

Yes.

I just talked to Ivan about some additional complexity in copying a repository. The result is that there's a new user story for additional functionality (https://dasch.myjetbrains.com/youtrack/issue/DSP-398). This would also require being able to specify:

  • a custom value creation date when adding a new version of a value
  • a custom delete date when marking a value as deleted

Do you want to do those in this PR?

@SepidehAlassi
Copy link
Contributor

Do you want to do those in this PR?

No, I prefer to do it in a separate PR.

@benjamingeer
Copy link
Author

I prefer to do it in a separate PR.

OK then I think this one is good to merge! You can approve it yourself. :)

@SepidehAlassi
Copy link
Contributor

I prefer to do it in a separate PR.

OK then I think this one is good to merge! You can approve it yourself. :)

Thanks for reviewing it!

@SepidehAlassi SepidehAlassi merged commit 135b039 into develop Jun 10, 2020
@SepidehAlassi SepidehAlassi deleted the wip/DSP-159-custom-iris branch June 10, 2020 12:32
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.

None yet

2 participants