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

Jump to bottom

[CAMEL-15253] camel-oaipmh - basic implementation #3934

Merged
merged 4 commits into from Aug 13, 2020
Merged

[CAMEL-15253] camel-oaipmh - basic implementation #3934

merged 4 commits into from Aug 13, 2020

Conversation

@juanksega
Copy link
Contributor

@juanksega juanksega commented Jun 18, 2020 •
edited

Google Summer of Code 2020:

Implementation of a camel-oaipmh component.

Student: Juan Segarra
Mentors:
Denis Istomin
Zoran Regvart
Andrea Cosentino

Documentation:
https://camel.apache.org/components/latest/oaipmh-component.html

Camel Examples:
apache/camel-examples#13

Spring Boot Starter:
apache/camel-spring-boot#143

Jira:
https://issues.apache.org/jira/browse/CAMEL-15253

Proposal:
https://docs.google.com/document/d/1zrPhEnhj5gc3VIPRWYXz7n6kJqrIjLbDHRRrCa5wwsg/edit?usp=sharing

Blog:
https://medium.com/@juanksegarraf/implementation-of-a-component-for-the-oai-pmh-protocol-in-apache-camel-94d98ca6908f

Mail threads:
http://camel.465427.n5.nabble.com/camel-oaipmh-quirks-mode-tp5878543.html
http://camel.465427.n5.nabble.com/camel-oaipmh-HTTPS-and-endpoint-schema-tp5871118.html
http://camel.465427.n5.nabble.com/GSoC-2020-Project-suggestion-OAI-PMH-tp5858089.html

*Part of this code was imported from: https://github.com/cbadenes/camel-oaipmh

[ ] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful subject line and body.
[ ] If you're unsure, you can format the pull request title like [CAMEL-XXX] Fixes bug in camel-file component, where you replace CAMEL-XXX with the appropriate JIRA issue.
[ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[ ] Run mvn clean install -Psourcecheck in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
Below are the contribution guidelines:
https://github.com/apache/camel/blob/master/CONTRIBUTING.md
@DenisIstomin DenisIstomin self-requested a review Jun 19, 2020
@juanksega
Copy link
Contributor Author

@juanksega juanksega commented Jun 26, 2020

Hi @DenisIstomin @zregvart @oscerd

I pulled the last commits from the master branch of apache/camel, also I added a Producer implementation and some tests. Please let me know if you have suggestions.

@bedlaj
Copy link
Contributor

@bedlaj bedlaj commented Jun 27, 2020

Good start. Since this is draft, i will give a few suggestions, instead of full review.

  • Please avoid using lombok there are many good reasons to not using that.
  • Use jsoup and joda-time versions provided in parent pom.
  • In final version, there must be all remote http calls removed, instead setup mock or use testcontainers.
  • Thread.sleep in tests can be replaced with proper assertions.
  • Never use assert keyword in unit tests.
  • Do not use @author annotations in javadoc.
  • Is safe to assume that response will be always UTF-8 and in Zulu timezone?
  • Headers should follow CamelOaimph* pattern, insead of oaimph.*
  • Endpoint class should contain some field annotated with @UriPath
  • For consumer use AsyncCallback in AsyncProcessor instead of try-catch-finally (Look eg to WsConsumer for inspiration)
  • Is build-helper-maven-plugin required? I dont see any custom generated (re)sources.

And some obvious things, which I believe you know about.

  • Checkstyle violations
  • Missing licence headers
  • Logging to console instead of file
  • Missing Jira issue
  • Commented out UriEndpoint annotation (+category attribute)
  • Full build needed, to generate endpoint-dsl, component-dsl, ...

@bedlaj bedlaj changed the title camel-oaipmh: basic implementation [CAMEL-15253] camel-oaipmh - basic implementation Jul 4, 2020
juanksega added 3 commits Jul 26, 2020
@juanksega
[CAMEL-15253] camel-oaipmh - implementation
3cc8ca4
@juanksega
Documentation fix
2c23cde
@juanksega
Improvements:
2b1bdc9 
- Implement HTTPS test.
- Add parameter: ignoreSSLWarnings
- Regen
@juanksega juanksega marked this pull request as ready for review Jul 30, 2020
omarsmak
omarsmak requested changes Jul 31, 2020
View changes
Copy link
Member

@omarsmak omarsmak left a comment

Thank you @juanksega for the PR! I have gave some comments and tips to improve this component further.

components/camel-oaipmh/pom.xml Outdated
</plugin>

<!-- generate camel meta-data -->
<plugin>
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

You don't need this plugin, is already included in the parent pom of the components.

components/camel-oaipmh/pom.xml Outdated
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

As well here

components/camel-oaipmh/pom.xml Outdated
<properties>
</properties>

<dependencyManagement>
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

You already having the parent dependency, I don't think you need to add the Camel BOM here

components/camel-oaipmh/pom.xml Outdated
<build>
<defaultGoal>install</defaultGoal>

<plugins>
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

and here

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/handler/Harvester.java Outdated
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.security.KeyManagementException;
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

I see these imports are not used here

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/utils/OAIPMHHttpClient.java Outdated

public String doRequest(URI baseURI, String verb, String set, String from, String until, String metadataPrefix, String token, String identifier) throws IOException, URISyntaxException, Exception {
CloseableHttpClient httpclient = getCloseableHttpClient();
try {
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

You can replace it with:

Suggested change
try {
try (CloseableHttpClient httpclient = getCloseableHttpClient()) {

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/utils/OAIPMHHttpClient.java Outdated

if (baseURI.getQuery() != null && !baseURI.getQuery().isEmpty()) {
for (String param : baseURI.getQuery().split("&")) {
builder.addParameter(param.split("=")[0], param.split("=")[1]);
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

I am just wondering, do we need to check that we really have two params split with =?

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/utils/OAIPMHHttpClient.java Outdated
};
String responseBody = httpclient.execute(httpget, responseHandler);

// String uri = requestLine.getUri();
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

unnecessary comments here

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/utils/OAIPMHHttpClient.java Outdated
builder.build());
return HttpClients.custom().setSSLSocketFactory(
sslsf).build();
} catch (Exception ex) {
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

If you are catching the exception and then throw it again, makes no sense. However, I don't feel comfortable of throwing generic Exception here, please be specific on which exception you want to throw here

components/camel-oaipmh/src/main/java/org/apache/camel/oaipmh/utils/OAIPMHHttpClient.java
protected CloseableHttpClient getCloseableHttpClient() throws Exception {
if (isIgnoreSSLWarnings()) {
try {
SSLContextBuilder builder = new SSLContextBuilder();
Copy link
Member

@omarsmak omarsmak Jul 31, 2020

Choose a reason for hiding this comment

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

This looks deprecated.

@juanksega
Improvements:
305b642 
- Remove unnecessary comments.
- Add constants file.
- Remove deprecated code.
@juanksega
Copy link
Contributor Author

@juanksega juanksega commented Aug 7, 2020

Hi @bedlaj @omarsmak @DenisIstomin

Thanks so much for your feedback. I think I fixed all your suggestions. Please, let me know if you have other suggestions.

oscerd
oscerd approved these changes Aug 7, 2020
View changes
Copy link
Contributor

@oscerd oscerd left a comment

LGTM, I think this is ready to merge.

bedlaj
bedlaj approved these changes Aug 7, 2020
View changes
Copy link
Contributor

@bedlaj bedlaj left a comment

LGTM. I see some unresolved suggestions, but these should not be blocking the merge:

@bedlaj: For consumer use AsyncCallback in AsyncProcessor instead of try-catch-finally (Look eg to WsConsumer for inspiration)

@omarsmak: I don't think you need to throw all these exception since you are throwing Exception.

DenisIstomin
DenisIstomin approved these changes Aug 7, 2020
View changes
Copy link
Member

@DenisIstomin DenisIstomin left a comment

LGTM, nice job @juanksega

NIT: inconsistent code comments formatting

omarsmak
omarsmak approved these changes Aug 10, 2020
View changes
Copy link
Member

@omarsmak omarsmak left a comment

LGTM, good job @juanksega ! I guess we can merge this.

@omarsmak
Copy link
Member

@omarsmak omarsmak commented Aug 13, 2020

@oscerd we shall merge this one?

@oscerd
Copy link
Contributor

@oscerd oscerd commented Aug 13, 2020

Yeah, let's merge and revisit and improving later

@omarsmak omarsmak merged commit 92c4b75 into apache:master Aug 13, 2020
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omarsmak omarsmak

@oscerd oscerd

@DenisIstomin DenisIstomin

@bedlaj bedlaj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@juanksega @bedlaj @omarsmak @oscerd @DenisIstomin