Skip to content

Commit

Permalink
NEW: Add 'get' method to make arbitrary requests
Browse files Browse the repository at this point in the history
  • Loading branch information
@vlad-x
vlad-x committed May 13, 2014
1 parent 498f889 commit 5e1edd4
Showing 1 changed file with 28 additions and 1 deletion.
29 changes: 28 additions & 1 deletion lib/solr.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ Client.prototype.search = function(query,callback){

/**
* Search documents matching the `query`
*
*
* Spellcheck is also enabled.
*
* @param {Query|String} query
Expand All @@ -436,6 +436,33 @@ Client.prototype.spell = function(query,callback){
return self;
}

/**
* Make an arbitraty get request to Solr
*
* @param {Path|String} Path
* @param {Query|String} query
* @param {Function} callback(err,obj) - a function executed when the Solr server responds or an error occurs
* @param {Error} callback().err
* @param {Object} callback().obj - JSON response sent by the Solr server deserialized
*
* @return {Client}
* @api public
*/

Client.prototype.get = function(path,query,callback){
var self = this;
// Allow to be more flexible allow query to be a string and not only a Query object
var parameters = query.build ? query.build() : query;
this.options.fullPath = [this.options.path,this.options.core,path + '?' + parameters + '&wt=json']
.filter(function(element){
if(element) return true;
return false;
})
.join('/'); ;
queryRequest(this.options,callback);
return self;
}

/**
* Create an instance of `Query`
*
Expand Down

6 comments on commit 5e1edd4

@marc-portier
Copy link

Choose a reason for hiding this comment

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

@vladif I wasn't around here when you first made this request, so excuse my ignorance.

AFAICS this only introduces an extra path argument compared to the available search() method, right?
This leaves me with a number of questions/remarks:

  1. What is your use case for this, and can't you just handle this through creating a client with the correct core specified?
  2. Should we then not make this available through the search() itself - supporting both search(query, callback) and search(path,query,callback) by checking the arguments-list? --> to be honest I would rather keep .get() to support the new solr4 /get feature (as mentioned in Provide support for solr 4 real time get lbdremy/solr-node-client#88)
  3. If however you really want to get under the skin of this client for more power/control, then it might be more useful to consider a path of exposing more of the underlaying updateRequest/queryRequest functions of the client?

@vlad-x
Copy link
Owner Author

@vlad-x vlad-x commented on 5e1edd4 Jul 19, 2014

Choose a reason for hiding this comment

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

Hi Marc,

  1. There is no way to make arbitrary HTTP GET requests in the original solr-client, but since you can configure Solr to have request handlers with any path, this is IMHO a useful addition.
  2. The search() method has a hardcoded path this.options.path,this.options.core,'select?', and I don't think it would be correct to make any changes to the original API.
  3. May be, but I thought I'd stick with having it similar to other methods, so that it is consistent with the rest of the code.

@marc-portier
Copy link

Choose a reason for hiding this comment

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

Vlad, thx for the clarification.

  1. Not to act dumb, but to make sure we're not talking at cross purposes: could you provide a (or some) simple example(s), hopefully useful as test-case(s)?
  2. I was suggesting to augment/extend search(query, callback) to maybe search(query, custom_path, callback) which would not need to break the API I think - but I think I was overlooking that your goal is not to do searches (/select) at all, right? You want to execute other handlers then the ones we currently have (i.e. update-select-ping)? Maybe handle(custom_handler_path,params,callback) then comes closer to the semantics of what you need/want? (still keeping get() available for Provide support for solr 4 real time get lbdremy/solr-node-client#88)
  3. I share your concern for consistency, my suggestions was obviously making the wrong assumptions about your use case. Still, following that line of thinking, it might be more consistent to just add the extra handlers to the client (just as Provide support for solr 4 real time get lbdremy/solr-node-client#88 is suggesting to do with .get(ids) to handle /get?ids=...) - this could of course be possible next to a generic handle() as proposed?

@vlad-x
Copy link
Owner Author

@vlad-x vlad-x commented on 5e1edd4 Jul 19, 2014

Choose a reason for hiding this comment

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

  1. We needed to configure different suggesters (https://wiki.apache.org/solr/Suggester) on different paths, e.g. /suggestword and /suggestphrase and there was no way to do it with the original library
    2-3. If you are concerned about naming, and want to reserve "get" for the real time get, we can of course rename it, though I don't see any problem just using it it as solr.get('/get', ..., function()...) 😄

@marc-portier
Copy link

Choose a reason for hiding this comment

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

LOL. Maybe we should let @lbdremy clarify which naming he thinks best fits the way he intended the library then?

Side-question for my understanding: how are these suggesters similar of different to what .spell() is doing on the node-client?

Anyway the fact that you needed multiple variants of suggesters surely show that in the brother sense just whatever custom handler that was defined in one's specific solr instance must be easily-callable. So now I really get your need.

One step further: we should probably also consider the need for people to send off POST requests to custom handlers too? In that case my earlier proposed handler() would either need a method (first) argument or would naturally fall apart into handleGet() versus handlePost(). Eurhm, yes, obviously and deliberately not get() and post() :)

Just to make clear: I only found your commit through a search for 'get' on this repo prior to starting on my lbdremy#88 implementation. I choose to make contact to avoid issues or unexpected side-effects on either side. But (as you stated yourself) we should balance for 'clarity of the API' as well...

As soon as we iron this out, I'll be happy to implement whatever we can agree upon as part of lbdremy#88

@vlad-x
Copy link
Owner Author

@vlad-x vlad-x commented on 5e1edd4 Jul 19, 2014

Choose a reason for hiding this comment

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

Well, this whole thing is so simple, and either implementation is quite trivial, so I honestly don't see much sense arguing about naming conventions and semantics 😃 I'd suggest you to just make a pull request with your implementation, I'm okay if any other code gets merged instead of mine, we can easily adapt our code to use any API. We're now using the library from my fork, however, we'll just change our code once any implementation of this functionality is available in the original library.

Regarding your question about .spell() I actually haven't checked how it works, but I suppose it is very similar, and differs just by some minor parameters.

Please sign in to comment.