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

Implement support for migrating RDF serializations other than turtle #61

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

cjcolvar
Copy link
Contributor

@cjcolvar cjcolvar commented Feb 4, 2025

What does this Pull Request do?

The upgrade-utils has an option for source rdf language (source-rdf) but in reality only turtle was supported. This PR adds support for other rdf serializations and tests with ntriples.

Jira Ticket

https://fedora-repository.atlassian.net/browse/FCREPO-3989

What's new?

  • Added getSrcRdfExt() convenience method to Config
  • In F47ToF5UpgradeManager:
    • Replaced TURTLE_EXTENSION with new method
    • Removed TURTLE_EXTENSION contsant
    • Replaced hard-coded Lang.TTL references to getSrcRdfLang()
  • DRY'd up F5ToF6UpgradeManager by using new method
  • Added tests and test fixtures for ntriples

How should this be tested?

Try exporting from fedora 4 or 5 in ntriples (or another rdf serialization) and running the upgrade utils.

Interested parties

@whikloj

@whikloj
Copy link
Contributor

whikloj commented Feb 6, 2025

Thanks for this @cjcolvar.

I'm having a little trouble with my 4.7.5 export, it seems to be lacking some information. But while I figure that out, could I get you to add a better test of the -r, --source-rdf argument. Currently if you pass a bad one (I was trying NT and NTRIPLES to begin) we just default to text/turtle and move on.

if (cmd.hasOption("source-rdf")) {
config.setSrcRdfLang(RDFLanguages.contentTypeToLang(cmd.getOptionValue("source-rdf")));
}

Something like

if (cmd.hasOption("source-rdf")) {
    final Lang lang = RDFLanguages.contentTypeToLang(cmd.getOptionValue("source-rdf"));
    if (lang == null) {
        printHelpAndExit(format("invalid RDF content-type (%s) provided", cmd.getOptionValue("source-rdf")),
            configOptions);
    }
    config.setSrcRdfLang(lang);
}

Copy link
Contributor

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

@cjcolvar I also figured out part of my issue, which might be that there are more spots assuming text/turtle in this codebase.
https://fedora-repository.atlassian.net/browse/FCREPO-3992

I'm good with this work, it does was it says. If you could add the rdf type check so people don't specify something and just get turtle then I'll merge.

But just be aware that the Fedora 4 to 5 "upgrade" might look like it is working but doesn't. It then fails to allow you to upgrade from Fedora 5 to 6.

@cjcolvar
Copy link
Contributor Author

cjcolvar commented Feb 7, 2025

Thanks @whikloj for the quick review! I added the check you suggested.

FWIW I think my upgrade to 6 worked correctly with these changes, but maybe my dataset is different from yours and wasn't hitting the issue you are seeing.

Copy link
Contributor

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

👍

@whikloj whikloj merged commit 8082e5a into fcrepo-exts:main Feb 13, 2025
3 checks passed
@cjcolvar cjcolvar deleted the src_rdf_lang branch February 14, 2025 14:31
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