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

migrate legacy uuid identifier #274

Open
wants to merge 13 commits into
base: dspace-cris-2023_02_x
Choose a base branch
from

Conversation

floriangantner
Copy link

@floriangantner floriangantner commented Feb 20, 2022

References

Add references/links to any related issues or PRs. These may include:

  • Fixes #[issue-number]

Description

Migrate existing UUID from Dspace Cris 5.X to entity uuid in dspace-cris7. Benefit: once assigned and shared uuid won't change after the migration.

Instructions for Reviewers

Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.

List of changes in this PR:

  • migration procedure: write uuid of object being migrated into imp_record.uuid column
  • database: added another column uuid in imp_record containing the uuid. It's not unique because there might be several updates available.
  • ItemImportOA:
    • during import (only on add; not on update/delete actions)
    • use the predefined uuid when some item is added.
    • if the uuid is invalid or already existing, some SQL-Exception should be thrown
    • The workspaceitem is generated with this value as the predefined uuid
  • added simple integration test

Include guidance for how to test or review your PR.

  • integration tests fail currently on dspace-server-webapp, thus waiting until these tests are fixes correctly on upstream.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

changes migration procedures to import the uuid of the object into
imp_record.handle . Copy ItemImport methods and remove handle support,
but treat the handle value in the table as the predefined uuid to set
for the newly created workspaceitem.
when handle of item contains some parsable uuid, use this value as the predefineduuid of the item being generated. Makes the changes on the import class more obvious
@floriangantner floriangantner marked this pull request as ready for review December 7, 2022 12:27
@steph-ieffam
Copy link

Hi @floriangantner, thanks for your pull request we have reviewed it and we think the feature is great but at this moment we're not able to merge this PR since we think some more changes should be included.
As I said the feature is fine for us, but we prefer adding a new column on the imp_record table for this purpose rather than reusing the handle column since this behavior might lead to confusion.

Said that, could you kindly update the PR with the needed changes?

Thanks

@steph-ieffam steph-ieffam self-assigned this Apr 18, 2023
@steph-ieffam steph-ieffam self-requested a review April 18, 2023 10:18
@steph-ieffam steph-ieffam marked this pull request as draft April 18, 2023 10:40
Copy link

@steph-ieffam steph-ieffam left a comment

Choose a reason for hiding this comment

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

Hi @floriangantner, thanks for your pull request we have reviewed it and we think the feature is great but at this moment we're not able to merge this PR since we think some more changes should be included.
As I said the feature is fine for us, but we prefer adding a new column on the imp_record table for this purpose rather than reusing the handle column since this behavior might lead to confusion.

Said that, could you kindly update the PR with the needed changes?

Thanks

@floriangantner floriangantner marked this pull request as ready for review August 21, 2023 14:28
@floriangantner floriangantner changed the base branch from dspace-cris-7 to main-cris February 20, 2024 12:39
@floriangantner
Copy link
Author

🎂 Happy Birthday PR, updated to recent 2023.02.02 Release and fixed Problem with uuid mapping breaking tests

@floriangantner floriangantner changed the base branch from main-cris to dspace-cris-2023_02_x April 9, 2024 09:56
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.

2 participants