added test for DISABLED digest type to FoxML digest Validation #190

Surfrdan · 2022-09-20T15:00:48Z

The title of this pull-request should be a brief description of what the pull-request fixes/improves/changes. Ideally 50 characters or less.

JIRA Ticket: https://fedora-repository.atlassian.net/browse/FCREPO-3847

What does this Pull Request do?

Adds a test for the DISABLED digest type and continues execution without validation if true.

What's new?

To the best of my understanding, this is a valid use case that was not accounted for previously. A Sanity check would be appreciated though.

How should this be tested?

run migration utils against FoxML inline XML objects with and without digests

Additional Notes:

Any additional information that you think would be helpful when reviewing this PR.

whikloj · 2022-09-20T15:49:59Z

Would be good to add an actual test to the tool to ensure we don't break this in the future.

Surfrdan · 2022-09-20T16:23:25Z

Agreed on the test. This PR also now kicks the can down the road to fail at the validation stage in fcrepo-storage-ocfl src/main/java/org/fcrepo/storage/ocfl/validation/ValidationUtil.java so we need to consider how to handle this sitation before merging this

mikejritter · 2022-10-07T22:45:39Z

At a quick glance this looks like it will work. I updated a managed datastream to set the checksum type to DISABLED and was able to run the migration, validation, and reindexing without a problem. I still want to check the migrator a little more to make sure it will be ok (it looks like if no digest is found on a datastream it's ignored in the ArchiveGroupHandler) and maybe run against external content as well. Note that it also appears the migrator will give all datastreams a sha512 digest so ocfl remains stable.

whikloj · 2022-10-08T14:57:51Z

That seems different from what @Surfrdan was experiencing. Where he could complete the migration but the reindexing would fail.

Surfrdan · 2022-10-10T10:36:49Z

I'm building a tiny test batch with the offending object so I can re-test this quickly and identify the exact issue again.

Surfrdan · 2022-10-10T11:08:20Z

I've updated the Jira ticket with the full stack trace and attacheds the offending FoxML

dbernstein · 2022-11-16T17:11:15Z

@Surfrdan : What is the status on this ticket? Re your comment above: Were you going to add an integration test to test the behavior?

Agreed on the test. This PR also now kicks the can down the road to fail at the validation stage in fcrepo-storage-ocfl src/main/java/org/fcrepo/storage/ocfl/validation/ValidationUtil.java so we need to consider how to handle this sitation before merging this

Surfrdan · 2022-11-16T17:57:05Z

@dbernstein Would it be sufficient to modify the input FoxML from one of the existing tests to represent an EXTERNAL datastream with a DISABLED and unreachable URL ?

Something like https://github.com/fcrepo-exts/migration-utils/blob/main/src/test/resources/legacyFS/objects/2015/0430/16/01/example_1#L141 for example

Surfrdan · 2022-11-22T18:13:27Z

I've decided to build by own test with the NLW data which failed in the first place. I'm just going to get clearance to use the data before I commit it to the repo though.

Surfrdan · 2022-11-25T12:00:14Z

let me fix those up. I can't run checkstyle from behind a corporate proxy unfortunately. Sorry.

… parse tge datastream dir

mikejritter

@Surfrdan This looks mostly good, I just have one question below from some behavior that I believe changed from the initial PR. Also, just wanted to note that the migrator will generate sha512 sums for all ocfl objects, so we don't need to worry about anything missing from the Fedora 6/ocfl side of things.

mikejritter · 2023-01-09T19:17:13Z

src/main/java/org/fcrepo/migration/foxml/FoxmlInputStreamFedoraObjectProcessor.java

@@ -468,6 +474,11 @@ private String extractInlineXml() throws XMLStreamException {

        private void validateInlineXml() {
            if (isInlineXml && contentDigest != null && StringUtils.isNotBlank(contentDigest.getDigest())) {
+
+                if (StringUtils.equals(contentDigest.getType(), "DISABLED")) {
+                    throw new RuntimeException("DISABLED digest. Skipping digest validation");


I'm wondering if we want to throw an exception here or not. As I understood the problem initially, it seemed like we wanted to continue migration of the pids with disabled digests because it was a feature of Fedora 3. If so, we could just log this instead.

I went the exception route to assist with building an integration test case but if you think logging would be sufficient I can revert that change.

I think logging then return to skip the checksum validation is acceptable. For the integration test we can switch to check if the migration was successful. It looks like the InlineXmlIT does something similar, but for this case we can check if the resources exist in the ocfl object instead of computing checksums.

I've now made those changes as suggested @mikejritter

mikejritter · 2023-01-17T15:47:26Z

src/test/java/org/fcrepo/migration/DisabledDigestIT.java

+    public void testMigrateObjectWithExternalDatastreamAndDisabledDigest() throws Exception {
+        setup("inline-disabled-it");
+        migrator.run();
+        final var migratedAuditPath = "target/test/ocfl/inline-disabled-it/storage/8f8/e55/54c/" +


It would be good to use the OCFL classes to look for the audit resource here. fcrepo-storage-ocfl contains the OcflObjectSession which can be used to check, though it doesn't look like we have javadocs up for that module.

Thanks for pointing me in the right direction. I've uipdatedf the IT

…session object

…repo-exts#189) Resolves: https://fedora-repository.atlassian.net/browse/FCREPO-3836

Resolves: https://fedora-repository.atlassian.net/browse/FCREPO-3863

Bumps [woodstox-core](https://github.com/FasterXML/woodstox) from 6.2.3 to 6.4.0. - [Release notes](https://github.com/FasterXML/woodstox/releases) - [Commits](FasterXML/woodstox@woodstox-core-6.2.3...woodstox-core-6.4.0) --- updated-dependencies: - dependency-name: com.fasterxml.woodstox:woodstox-core dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

… parse tge datastream dir

…session object

…tils into FCREPO-3847

mikejritter

@Surfrdan looks good now 👍

whikloj · 2023-02-07T15:01:54Z

src/test/java/org/fcrepo/migration/DisabledDigestIT.java

+        setup("inline-disabled-it");
+        migrator.run();
+        final var session = sessionFactory.newSession("info:fedora/llgc-id:1591190");
+        assertTrue(session.containsResource("info:fedora/llgc-id:1591190/AUDIT"));


Shouldn't we be testing for the existence of one of the datastreams with a DISABLED digest?

Fixed the ID to look for a DISABLED digest

whikloj

👍

added test for DISABLED digest type

9b7f3ab

Adding Integration Test and a new exceptin for disabled digests

41e8835

Dan Field and others added 8 commits November 28, 2022 09:44

checkstyle fixes

c6be76a

changing index to match inline-it et al

13f3ce3

seperating test index in case of clash

bb13c9e

Merge remote-tracking branch 'upstream/main' into FCREPO-3847

464ded8

fixing issues with the disabled integration test

00d36d3

removing superflous log message as we throw an exception now

c72a992

git wasn't adding an empty dir. used .gitkeep and now the test should…

99d22f4

… parse tge datastream dir

.gitkeep broke the indexer. copying a datastream from another test

d7066ac

mikejritter reviewed Jan 9, 2023

View reviewed changes

reverting to logging rather than throwing exception

888afb2

mikejritter reviewed Jan 17, 2023

View reviewed changes

Surfrdan and others added 7 commits January 19, 2023 14:57

improved DISABLED fixity Integration Test to use fcrepo-storage-ocfl …

8cd2a06

…session object

Switch from assembly to shade plugin to correct Jena re-packaging (fc…

5b3651c

…repo-exts#189) Resolves: https://fedora-repository.atlassian.net/browse/FCREPO-3836

Alter test assertion to allow for ocfl 1.0 and 1.1 (fcrepo-exts#192)

6c4f373

Resolves: https://fedora-repository.atlassian.net/browse/FCREPO-3863

Increment version to 6.3.1-SNAPSHOT

d391ed4

fixing issues with the disabled integration test

0753f0b

removing superflous log message as we throw an exception now

db3fbd9

Surfrdan added 5 commits January 19, 2023 15:14

git wasn't adding an empty dir. used .gitkeep and now the test should…

af793ec

… parse tge datastream dir

.gitkeep broke the indexer. copying a datastream from another test

8f7ddfd

reverting to logging rather than throwing exception

e28c397

improved DISABLED fixity Integration Test to use fcrepo-storage-ocfl …

1065504

…session object

Merge branch 'FCREPO-3847' of https://github.com/Surfrdan/migration-u…

0116ec4

…tils into FCREPO-3847

mikejritter approved these changes Jan 19, 2023

View reviewed changes

whikloj reviewed Feb 7, 2023

View reviewed changes

fixed test to look for a DISABLED digest

0009f78

whikloj approved these changes Feb 16, 2023

View reviewed changes

whikloj merged commit bad1e17 into fcrepo-exts:main Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added test for DISABLED digest type to FoxML digest Validation #190

added test for DISABLED digest type to FoxML digest Validation #190

Surfrdan commented Sep 20, 2022

whikloj commented Sep 20, 2022

Surfrdan commented Sep 20, 2022

mikejritter commented Oct 7, 2022

whikloj commented Oct 8, 2022

Surfrdan commented Oct 10, 2022

Surfrdan commented Oct 10, 2022

dbernstein commented Nov 16, 2022

Surfrdan commented Nov 16, 2022

Surfrdan commented Nov 22, 2022

Surfrdan commented Nov 25, 2022

mikejritter left a comment

mikejritter Jan 9, 2023

Surfrdan Jan 10, 2023

mikejritter Jan 10, 2023

Surfrdan Jan 12, 2023

mikejritter Jan 17, 2023

Surfrdan Jan 19, 2023

mikejritter left a comment

whikloj Feb 7, 2023

Surfrdan Feb 16, 2023

whikloj left a comment

added test for DISABLED digest type to FoxML digest Validation #190

added test for DISABLED digest type to FoxML digest Validation #190

Conversation

Surfrdan commented Sep 20, 2022

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

whikloj commented Sep 20, 2022

Surfrdan commented Sep 20, 2022

mikejritter commented Oct 7, 2022

whikloj commented Oct 8, 2022

Surfrdan commented Oct 10, 2022

Surfrdan commented Oct 10, 2022

dbernstein commented Nov 16, 2022

Surfrdan commented Nov 16, 2022

Surfrdan commented Nov 22, 2022

Surfrdan commented Nov 25, 2022

mikejritter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejritter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whikloj left a comment

Choose a reason for hiding this comment