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

repository: added support for tx calls #31

Closed
wants to merge 2 commits into from

Conversation

gushakov
Copy link
Contributor

@gushakov gushakov commented Oct 7, 2015

Modified FedoraRepositoryImpl slightly to add some useful calls to RESTfull API to deal with transactions. Based on the description given at https://wiki.duraspace.org/display/FEDORA4x/RESTful+HTTP+API+-+Transactions

The idea is to be able to do something like this.

repo.startTransaction();
try {
  repo.createObject(path);
  // create datastreams, related objects, update properties, etc.
}
catch (Exception e) {
  // roll back if something goes wrong
  repo.rollbackTransaction();
}
repo.commitTransaction();
// or

The transaction IDs (with tx: prefix) are stored in ThreadLocal<String> attribute of the static utility class.

@awoods
Copy link
Member

awoods commented Oct 7, 2015

@gushakov, Thank you for this. Can you please submit an iCLA:
https://wiki.duraspace.org/display/DSP/Contributor+License+Agreements

@mikedurbin or @escowles, I am on travel this week. Will you have some time to review this, and create a JIRA ticket?

@escowles
Copy link
Contributor

escowles commented Oct 7, 2015

I've created https://jira.duraspace.org/browse/FCREPO-1762 and should have time to review this today or tomorrow.

// check if path needs to be prepended with a slash
final String txId = TransactionIdHolder.getCurrentTransactionId();
return (txId == null || path.contains(TX))
? path : (path.startsWith("/") ? "/" + txId + path : txId + "/" + path);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to avoid nested ?: conditions -- they are hard enough to read without being nested.

@escowles
Copy link
Contributor

escowles commented Oct 7, 2015

@gushakov: This looks good -- I have a few minor comments, and this looks ready to merge once those are addressed and your CLA is signed.

@gushakov
Copy link
Contributor Author

gushakov commented Oct 9, 2015

Just sent my iCLA.

As suggested:

  • moved ThreadLocal tx id holder variable into FedoraRepositoryImpl
  • prepending for the null containerPath in createResource
  • rollback whitespace-only changes
  • refactored nested ? checks

@escowles
Copy link
Contributor

escowles commented Oct 9, 2015

👍 -- this looks ready to merge as soon as your CLA is signed processed.

}

} catch (final Exception e) {
LOGGER.error("could not encode URI parameter", e);
Copy link
Member

Choose a reason for hiding this comment

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

Please do not dump exception stacktraces into the log. Instead, print the e.getMessage().

@awoods
Copy link
Member

awoods commented Oct 19, 2015

Thanks for this, @gushakov. Could you please let me know what your https://jira.duraspace.org user-id is so that we can associate you to this ticket:
https://jira.duraspace.org/browse/FCREPO-1762

Rarian added a commit to Rarian/fcrepo4-client that referenced this pull request Dec 14, 2015
Instead of dumping a full stacktrace to the log, the convention within
Fedora is to instead append the Exception.getMessage() to the specified
error.

fcrepo4-labs#31 (comment)
Rarian added a commit to Rarian/fcrepo4-client that referenced this pull request Dec 14, 2015
Instead of dumping a full stacktrace to the log, the convention within
Fedora is to instead append the Exception.getMessage() to the specified
error.

fcrepo4-labs#31 (comment)
@awoods
Copy link
Member

awoods commented Dec 31, 2015

This work was completed with: #36

@awoods awoods closed this Dec 31, 2015
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

3 participants