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

Single level of abstraction / Better separation of concerns in query methods #280

Merged
merged 5 commits into from
Aug 30, 2021

Conversation

rudolfbyker
Copy link
Collaborator

Previously, we had .get and .post, which did something like this:

get:

  • Build a query.
  • If the URL is too large, throw everything away and call post.
  • Perform HTTP GET.

post:

  • Build a query.
  • Perform HTTP POST.

The problem with this is that neither GET nor POST what one would expect:

  • Neither are general HTTP GET or POST utilities.
  • They work specifically with Solr Query objects.
  • They expect JSON responses only.
  • They send their data as query strings only (even .post, which uses application/x-www-form-urlencoded).

Therefore this can be much better encapsulated as:

doQuery:

  • Take a Solr Query object, and prepare data for an HTTP query from that.
  • Decide whether GET or POST should be used (note that this behaviour comes from Search Solr using POST instead of GET #129 – I did not invent it)
  • Call doRequest.

doRequest:

  • Build the query.
  • Send the query.

This maintains a single level of abstraction per method, and therefore gives better separation of concerns and results in more understandable code.

@rudolfbyker rudolfbyker self-assigned this Aug 30, 2021
* Full URL to the handler.
*/
private getFullHandlerPath(handler: string): string {
let pathArray;
Copy link
Collaborator

Choose a reason for hiding this comment

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

path.resolve wouldn't do same thing in a simpler way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's possible. We can optimize in a next PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rudolfbyker rudolfbyker merged commit a8b980d into master Aug 30, 2021
@rudolfbyker rudolfbyker deleted the dev-single-level-of-abstraction branch August 30, 2021 13:50
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.

None yet

2 participants