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

Adds integration tests for http api. #44

Merged
merged 2 commits into from
Nov 1, 2017
Merged

Conversation

dbernstein
Copy link
Member

No description provided.

@dbernstein dbernstein requested a review from awoods October 31, 2017 19:58
@dbernstein
Copy link
Member Author

@awoods et al: I'm wondering if we should abandon FedoraLdpTest since we now have coverage in FedoraLdpTestIT

Copy link
Member

@awoods awoods left a comment

Choose a reason for hiding this comment

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

@@ -15,13 +15,21 @@
private JerseyLambdaContainerHandler<AwsProxyRequest, AwsProxyResponse> handler;

/**
* Initialise the request handler with the AWS Proxy
* Default Constructor
*/
public JerseyRequestHandler() {
Copy link
Member

Choose a reason for hiding this comment

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

Is the default constructor being used anywhere?
If not, we can probably remove it.

return internalURI.equals(rootURI);
}

private Container createRoot(final URI internalRootURI) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we are storing the rootURI, it may be safer to change this method to:

private Container createRoot() {
    return getContainerService().findOrCreate(rootURI);
}

final Container container = containerService.findOrCreate(objectURI);
final ContainerService containerService = getContainerService();
final URI resourceUri = createFromPath(externalPath);
//check that resource exists:
Copy link
Member

Choose a reason for hiding this comment

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

This comment is helpful... but it has drifted two lines apart from the related source code.


final URI newResourceUri = createFromPath(resourceUri.getPath() + slug);
Copy link
Member

Choose a reason for hiding this comment

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

Since the slug is optional, we should check for null. In the case that the slug is null, we need to generate a UUID for the new resource name.

In either case, the new resource should be: resourceUri.getPath() + / + newId

final URI newResourceUri = createFromPath(resourceUri.getPath() + slug);
final Container container = containerService.findOrCreate(newResourceUri);
final Model model = ModelFactory.createDefaultModel();
model.read(requestBodyStream, null, "TTL");
Copy link
Member

Choose a reason for hiding this comment

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

We can limit the input RDF type to turtle for now, but we should be supporting a range of RDF serialization as done here: https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-http-api/src/main/java/org/fcrepo/http/api/ContentExposingResource.java#L660-L664

Copy link
Member Author

Choose a reason for hiding this comment

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

@awoods : so no change here for now, yes?

Copy link
Member

Choose a reason for hiding this comment

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

I am ok with no change for now. I will create a GitHub issue.

* Updates comments on JerseyRequestHandler constructor
* Removes unused ApiGatewayResponse and Response classes
* Adds support for null slugs (and  adds a new integration test to test said support)
Copy link
Member

@awoods awoods left a comment

Choose a reason for hiding this comment

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

Looks good.
I will note that the integration tests still fail for me, but that is likely a local environment issue.

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.

3 participants