Skip to content

Commit

Permalink
Renaming setUpJMSBaseURIs to setUpJMSInfo, narrowing exception handli…
Browse files Browse the repository at this point in the history 
…ng, and using Gson instead of building JSON as a string
  • Loading branch information
@escowles
escowles committed Apr 1, 2015
1 parent 1227c61 commit 17f3ee6
Show file tree
Hide file tree
Showing 6 changed files with 19 additions and 8 deletions.
6 changes: 6 additions & 0 deletions fcrepo-http-api/pom.xml
View file
Expand Up @@ -67,6 +67,12 @@
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
<version>2.3</version>
</dependency>

<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
Expand Down
13 changes: 9 additions & 4 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraBaseResource.java
View file
Expand Up @@ -16,7 +16,9 @@
package org.fcrepo.http.api;

import com.google.common.annotations.VisibleForTesting;
import com.google.gson.JsonObject;
import com.hp.hpl.jena.rdf.model.Resource;
import org.apache.commons.lang.StringUtils;
import org.fcrepo.http.commons.AbstractResource;
import org.fcrepo.http.commons.api.rdf.HttpResourceConverter;
import org.fcrepo.kernel.models.FedoraResource;
Expand Down Expand Up @@ -77,14 +79,17 @@ public FedoraResource getResourceFromPath(final String externalPath) {
* @param uriInfo the uri info
* @param headers HTTP headers
**/
protected void setUpJMSBaseURIs(final UriInfo uriInfo, final HttpHeaders headers) {
protected void setUpJMSInfo(final UriInfo uriInfo, final HttpHeaders headers) {
try {
final URI baseURL = uriInfo.getBaseUri();
LOGGER.debug("setting baseURL = " + baseURL.toString());
final ObservationManager obs = session().getWorkspace().getObservationManager();
final String json = "{\"baseURL\":\"" + baseURL.toString() + "\","
+ "\"userAgent\":\"" + headers.getHeaderString("user-agent") + "\"}";
obs.setUserData(json);
final JsonObject json = new JsonObject();
json.addProperty("baseURL", baseURL.toString());
if (!StringUtils.isBlank(headers.getHeaderString("user-agent"))) {

This comment has been minimized.

Sign in to view

Copy link
@awoods

awoods Apr 6, 2015

In looking at Fedora logs, "userAgent" is:
"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/41.0.2272.76 Chrome/41.0.2272.76 Safari/537.36"

...is that really what we had in mind?

This comment has been minimized.

Sign in to view

Copy link
@escowles

escowles Apr 6, 2015

Author Contributor

I admit it's not pretty. But the UserAgent is definitely the standard way an HTTP client tells the server what software is connecting. If the userAgent isn't a browser (curl, Hydra, Islandora, custom application), then it should set a useful UserAgent too.

This comment has been minimized.

Sign in to view

Copy link
@awoods

awoods Apr 6, 2015

I was under the impression that we wanted messages to include the "user" that was performing a given action, i.e. "escowles".
Even with curl, I am seeing a userAgent of "curl/7.37.1"
What is this buying us?

In the case where there is no auth involved, I would expect to see "bypassAdmin" as the user.

This comment has been minimized.

Sign in to view

Copy link
@escowles

escowles Apr 6, 2015

Author Contributor

We want both the user and the userAgent. The userID is already available to the event system, so we don't need to pass it here (see 1227c61#diff-4e7f764f0d1ddf8237b09a6d066f4062R111). But the userAgent (like the baseURL) is only available at the REST API layer, so we have to capture it here and pass it to the event system using the observation manager.

This comment has been minimized.

Sign in to view

Copy link
@awoods

awoods Apr 6, 2015

If this is expected of the "userAgent" field and useful in the context of the Audit Service... ok.
json.addProperty("userAgent",headers.getHeaderString("user-agent"));
}
obs.setUserData(json.toString());
} catch ( Exception ex ) {
LOGGER.warn("Error setting baseURL", ex);
}
Expand Down
2 changes: 1 addition & 1 deletion fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLdp.java
View file
Expand Up @@ -132,7 +132,7 @@ public FedoraLdp(final String externalPath) {
*/
@PostConstruct
public void postConstruct() {
setUpJMSBaseURIs(uriInfo, headers);
setUpJMSInfo(uriInfo, headers);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraNodes.java
View file
Expand Up @@ -96,7 +96,7 @@ public FedoraNodes(final String externalPath) {
*/
@PostConstruct
public void postConstruct() {
setUpJMSBaseURIs(uriInfo, headers);
setUpJMSInfo(uriInfo, headers);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLdpTest.java
View file
Expand Up @@ -851,7 +851,7 @@ public void testSetUpJMSBaseURIs() throws RepositoryException {
doReturn(mockManager).when(mockWorkspace).getObservationManager();
final String json = "{\"baseURL\":\"http://localhost/fcrepo\",\"userAgent\":\"Test UserAgent\"}";

testObj.setUpJMSBaseURIs(getUriInfoImpl(), mockHeaders);
testObj.setUpJMSInfo(getUriInfoImpl(), mockHeaders);
verify(mockManager).setUserData(eq(json));
}
}
2 changes: 1 addition & 1 deletion fcrepo-jms/src/main/java/org/fcrepo/jms/headers/DefaultMessageFactory.java
View file
Expand Up @@ -100,7 +100,7 @@ public Message getMessage(final FedoraEvent jcrEvent,
LOGGER.warn("MessageFactory event UserData is empty!");
}

} catch ( final Exception ex ) {
} catch ( final RuntimeException ex ) {
LOGGER.warn("Error setting baseURL or userAgent", ex);
}

Expand Down

0 comments on commit 17f3ee6

Please sign in to comment.