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

add prefix check before update properties #921

Closed
wants to merge 2 commits into from

Conversation

nianma
Copy link
Contributor

@nianma nianma commented Oct 26, 2015

I don't have time to create new type exception. Also I would prefer some design this first like naming...

@nianma
Copy link
Contributor Author

nianma commented Oct 26, 2015

@@ -436,6 +436,19 @@ public void updateProperties(final IdentifierConverter<Resource, FedoraResource>

final Collection<IllegalArgumentException> errors = checkInvalidPredicates(request);

request.getPrefixMapping().getNsPrefixMap().forEach(
(k,v) -> { try {
LOGGER.info("451: prefix mapping is key -> value: " + k + "->" + v + "\n");
Copy link

Choose a reason for hiding this comment

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

Comments on your logging:

  • When using slf4j (which you are), you should take advantage of templating. For example, the above line should be:
    LOGGER.info("451: prefix mapping is key -> value: {} -> {}", k, v);
  • This should be debug not info
  • Please remove 451:, unless I am missing its purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

That better not be a reference to HTTP 451.

@awoods
Copy link

awoods commented Oct 27, 2015

Please create an integration test that demonstrates the issue before your functional updates, and proves the fix after your functional updates.
You can probably add a test method to:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-modeshape/src/test/java/org/fcrepo/integration/kernel/modeshape/FedoraResourceImplIT.java

/**
* For invalid namespace exceptions on CRUD actions for nodes/datastreams
*
* @author whikloj
Copy link

Choose a reason for hiding this comment

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

@author should be nianma

@awoods
Copy link

awoods commented Nov 29, 2015

Resolved with: 8057124

@awoods awoods closed this Nov 29, 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