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

implementing /contributions/count endpoint #121

Merged
merged 29 commits into from
Mar 19, 2021

Conversation

FabiKo117
Copy link
Contributor

@FabiKo117 FabiKo117 commented Feb 9, 2021

Description

Implementation of new endpoint /contributions/count

Corresponding issue

#115

New or changed dependencies

  • none

Checklist

@FabiKo117 FabiKo117 added this to the 1.4 milestone Feb 9, 2021
@FabiKo117 FabiKo117 self-assigned this Feb 9, 2021
@FabiKo117 FabiKo117 force-pushed the implement-contributions-count branch 3 times, most recently from 4022a0a to 8b09173 Compare February 19, 2021 10:14
@FabiKo117 FabiKo117 changed the title WIP: implementing /contributions/count endpoint implementing /contributions/count endpoint Feb 19, 2021
@FabiKo117 FabiKo117 added the waiting for review This pull request needs a code review label Feb 19, 2021
@FabiKo117 FabiKo117 force-pushed the implement-contributions-count branch from 5a6a3e1 to 0aeef82 Compare March 5, 2021 12:44
@FabiKo117 FabiKo117 added waiting for review This pull request needs a code review and removed waiting for review This pull request needs a code review labels Mar 5, 2021
Copy link
Contributor

@bonaparten bonaparten left a comment

Choose a reason for hiding this comment

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

I know that we should avoid boilerplate code, but let some classes of the contributions endpoints be used by classes of the users endpoints could be misleading. Here some points:

  • The UsersController.java uses the ContributionsExecutor.java
  • The ContributionsResult.java is called by classes of users endpoints like the UsersRequestExecutor.java
  • The method "count" in the class ContributionsExecutor is now used for users/count
  • The method writeCsvResponse in AggregateRequestExecutor.java uses ContributionsResults.java even if this class is supposed to be used only for /elements and /elements/_/groupBy/boundary endpoints.

Refactoring some classes and let them "extend" other in-between classes could be a solution.

//edit: adding a get test would be probably fine.

docs/endpoints.rst Outdated Show resolved Hide resolved
@FabiKo117
Copy link
Contributor Author

* The UsersController.java uses the ContributionsExecutor.java

This is because the /users endpoint also uses the contribution-view of the OSHDB, hence their processing is similar. We could think about renaming the executor classes then in a way that this is better reflected and cannot be mixed up with the name of the endpoint /contributions. This would be one thing that should be performed with the general refactoring including the extension of the AggregageRequestExecutor class, which is not working for every endpoint yet too.

* The ContributionsResult.java is called by classes of users endpoints like the UsersRequestExecutor.java

Same reason as described above.

* The method "count" in the class ContributionsExecutor is now used for users/count

Again same reason.

* The method writeCsvResponse in AggregateRequestExecutor.java uses ContributionsResults.java even if this class is supposed to be used only for /elements and  /elements/_/groupBy/boundary endpoints.

This is indeed not so nice code-quality wise. But this behavior did not get introduced with this branch. It was already before using the /users endpoint. But you are right here, this would also have to be refactored. Maybe we can create something like a GenericRequestExecutor that holds methods like writeCsvResponse which are used through different endpoint and processing structures?

Refactoring some classes and let them "extend" other in-between classes could be a solution.

//edit: adding a get test would be probably fine.

I don't understand what you mean with these two points. A test for what exactly?

@bonaparten
Copy link
Contributor

* The method writeCsvResponse in AggregateRequestExecutor.java uses ContributionsResults.java even if this class is supposed to be used only for /elements and  /elements/_/groupBy/boundary endpoints.

This is indeed not so nice code-quality wise. But this behavior did not get introduced with this branch. It was already before using the /users endpoint. But you are right here, this would also have to be refactored. Maybe we can create something like a GenericRequestExecutor that holds methods like writeCsvResponse which are used through different endpoint and processing structures?

We have this method also in ExecutionUtils.java. Yes, this could be a solution. Could you please change the javadoc upon AggregateRequestExecutor adding that it is used also for the contributions endpoints?

Refactoring some classes and let them "extend" other in-between classes could be a solution.
//edit: adding a get test would be probably fine.

I don't understand what you mean with these two points. A test for what exactly?

Like you already said with the class GenericRequestExecutor, I meant to create some superclasses and interfaces where needed in order to achieve more inheritance and structure.
I saw you added some tests for POST requests, you could also add some tests for GET requests.

@FabiKo117
Copy link
Contributor Author

@bonaparten I've updated the javadoc.

I saw you added some tests for POST requests, you could also add some tests for GET requests.

It does not matter what type of HTTP request is used in the tests, as the content of GET and POST is processed equally within the code. This was different in earlier versions of the code, but now there is no difference in the processing.

@FabiKo117 FabiKo117 changed the title implementing /contributions/count endpoint WIP: implementing /contributions/count endpoint Mar 17, 2021
@FabiKo117 FabiKo117 changed the title WIP: implementing /contributions/count endpoint implementing /contributions/count endpoint Mar 17, 2021
@FabiKo117 FabiKo117 requested a review from bonaparten March 18, 2021 10:42
bonaparten
bonaparten previously approved these changes Mar 18, 2021
@tyrasd tyrasd force-pushed the implement-contributions-count branch from 7cb42ad to 3886873 Compare March 18, 2021 13:31
@tyrasd tyrasd force-pushed the implement-contributions-count branch from 14d36fe to 5856e15 Compare March 18, 2021 13:38
@FabiKo117 FabiKo117 force-pushed the implement-contributions-count branch from 61fa070 to 89867d0 Compare March 18, 2021 16:24
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

I noticed that this new endpoint still supports the old types/keys/values syntax. I'd rather not have deprecated functionality included with new endpoints. Same as already discussed in #34 (review)

types, keys, values are not part of the set of permitted parameters for any /contributions endpoint
@FabiKo117 FabiKo117 requested review from tyrasd and bonaparten March 18, 2021 17:04
bonaparten
bonaparten previously approved these changes Mar 19, 2021
@tyrasd tyrasd mentioned this pull request Mar 19, 2021
6 tasks
since the current version does not yet support these requests ;)
docs/endpoints.rst Outdated Show resolved Hide resolved
tyrasd
tyrasd previously approved these changes Mar 19, 2021
@tyrasd tyrasd removed the waiting for review This pull request needs a code review label Mar 19, 2021
@tyrasd tyrasd force-pushed the implement-contributions-count branch from 1dd6d06 to 5a1793a Compare March 19, 2021 11:09
@tyrasd tyrasd merged commit b47b2b0 into master Mar 19, 2021
@tyrasd tyrasd deleted the implement-contributions-count branch March 19, 2021 11:16
tyrasd added a commit that referenced this pull request Apr 7, 2021
tyrasd added a commit that referenced this pull request Apr 7, 2021
fix code style issues introduced in PRs #162 and #121
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