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

added support for prefer headers along with unit and integration tests #31

Closed
wants to merge 2 commits into from
Closed

Conversation

acoburn
Copy link
Contributor

@acoburn acoburn commented Jan 6, 2015

This addresses https://jira.duraspace.org/browse/FCREPO-1273

Specifically, it allows users to control the Prefer headers sent to the fedora endpoint

@@ -26,6 +26,8 @@

public static final String FCREPO_TRANSFORM = "FCREPO_TRANSFORM";

public static final String HTTP_PREFER = "CamelHttpPrefer";
Copy link

Choose a reason for hiding this comment

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

Is there rationale for straying from the ALL_CAPS convention established by the previous three FCREPO headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of. I began using ALL_CAPS because I had seen that elsewhere, but other camel components seem to use the CamelComponentHeader convention, especially the HTTP headers. There is a common pattern to remove all HTTP headers with a line like

removeHeaders("CamelHttp*")

By following that convention, the CamelHttpPrefer header fits more easily into existing patterns.

Really, though, the other headers should be changed to:

CamelFcrepoTransform
CamelFcrepoBaseUrl
CamelFcrepoIdentifier

I would recommend changing those headers, too. The main question, though, is whether CamelHttpPrefer shouldn't instead by CamelFcrepoPrefer. And the more I think about it, the more I think it should be CamelFcrepoPrefer.

Copy link

Choose a reason for hiding this comment

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

Sounds good.

@awoods
Copy link

awoods commented Jan 7, 2015

As a general comment, more logging will make this codebase easier to trace and debug.

@@ -29,6 +33,20 @@

public static final String LDP = "http://www.w3.org/ns/ldp#";

public static final String SERVER_MANAGED = REPOSITORY + "ServerManaged";
Copy link

Choose a reason for hiding this comment

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

None of the below properties are used outside of tests.
Do we need to keep the additions to this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. It's possible that these String values would be used in running code, but I doubt that. The Map<String, String> is used in String FcrepoProducer.addPreferNamespace(String).

Changes include:
* Change visibility of certain protected methods to private in FcrepoProducer
* Change format of header values to follow Camel conventions (e.g. CamelFcrepoIdentifier)
* Remove unused constants
* Updated the documentation related to these changes (and fixed an incorrect link)
@awoods
Copy link

awoods commented Jan 7, 2015

Resolved with: 3ad67c8

@awoods awoods closed this Jan 7, 2015
@acoburn acoburn deleted the prefer-headers branch January 8, 2015 14:09
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

2 participants