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
Collaborator

@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

@SepidehAlassi SepidehAlassi commented Jun 4, 2020

@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
Collaborator Author

@benjamingeer benjamingeer commented Jun 4, 2020

Yes, good analysis!

@SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Jun 5, 2020

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

@SepidehAlassi SepidehAlassi commented Jun 9, 2020

@benjamingeer eventually the tests passed!

Copy link
Collaborator Author

@benjamingeer benjamingeer left a comment

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

@SepidehAlassi SepidehAlassi commented Jun 9, 2020

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 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
Collaborator Author

@benjamingeer benjamingeer left a comment

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
Collaborator Author

@benjamingeer benjamingeer commented Jun 10, 2020

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

@SepidehAlassi SepidehAlassi commented Jun 10, 2020

Do you want to do those in this PR?

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

@benjamingeer benjamingeer requested a review from SepidehAlassi Jun 10, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jun 10, 2020

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

@SepidehAlassi SepidehAlassi commented Jun 10, 2020

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
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Publish to Dockerhub
Details
@SepidehAlassi SepidehAlassi deleted the wip/DSP-159-custom-iris branch Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants