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

Separate concerns in the ReadOperation #201

Closed
ivansenic opened this issue Feb 28, 2023 · 2 comments · Fixed by #283
Closed

Separate concerns in the ReadOperation #201

ivansenic opened this issue Feb 28, 2023 · 2 comments · Fixed by #283
Assignees

Comments

@ivansenic
Copy link
Contributor

ivansenic commented Feb 28, 2023

Currently the ReadOperation serves as a base class for FindOperation and CountOperation. However, the count operation does not implement any of the abstract methods:

  @Override
  public Uni<FindResponse> getDocuments(QueryExecutor queryExecutor, String pagingState) {
    return Uni.createFrom().failure(new JsonApiException(ErrorCode.UNSUPPORTED_OPERATION));
  }

  @Override
  public ReadDocument getNewDocument() {
    throw new JsonApiException(ErrorCode.UNSUPPORTED_OPERATION);
  }

In addition the count related method countDocuments is used only by the CountOperation, thus is not needed at all in the base interface. This all looks like a bad design.

Tasks:

  • Count operation does not need to extend the read operation
    • Move count related method to the count operation
  • Same can be done for Find operation, effectivelly leaving Read operation as empty interface
  • In order for this to work the FilterableResolver class should be transitioned to FilterableResolverService and resolver can directly use this service to resolve the filter clause

@amorton @amorton Please review and approve issue.

@maheshrajamani
Copy link
Contributor

Imo changing the FilterableResolver as FilterableResolverService is not required.
Easier refactoring for the 2 methods, would be to add default implementation to return Unsupported_Operation exception in ReadOperation interface and remove it from CountOperation class. Even if any new implementation for this interface will be taken care of.

@ivansenic
Copy link
Contributor Author

We should never throw UnsupportedOperationException, it's the first sign of a bad design.

maheshrajamani added a commit that referenced this issue Mar 22, 2023
…the command needed a read to resolve the document.

Addresses concern in #201
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 a pull request may close this issue.

3 participants