query-builder refactoring

[joemfb]

This PR is the first step in a longer process of query-builder refactoring and improvement. As a first step, it deprecates custom functionality, replacing it with methods that match the official node-client-api query-builder. It moves additional functionality, specifically constraint-queries, to an ext property on the query-builder, to allow for compatibility as well as extension.

Additionally, it deprecates text() (which is only partially supported, and should simply be a property of a combined query) and updates propertiesFragment() to use the ML 8 syntax.

re #1 #11

/cc @grtjn

[grtjn]

Looks promising. But we definitely need to cover (q)text in some shape. We use it in all our demos, because that is where you put the search box string..

[grtjn]

I will also definitely need support for custom constraints, which is not covered yet I guess. That can be a separate PR though. Odd that node-client-api doesn't cover those by the way..

[joemfb]

re: qtext, definitely, but it needs to be part of a combined query, or a parameter to /v1/search:

• qtext property/element in combined query: http://docs.marklogic.com/guide/rest-dev/search#id_69918
• q param of /v1/search: http://docs.marklogic.com/REST/GET/v1/search

And this isn't supposed to just replace your PR; there's a lot of good stuff in there. This pull request is focused on refactoring for future compatibility, while maintaining backward compatibility. I want to work your contributions in, but it'll have to be spread over multiple releases.

Thoughts?

[grtjn]

Doesn't it make sense to have the query-builder also cover the combined query stuff? Then you could also consider just dropping the q param in favor of just always sending a POST body..

[joemfb]

Sure, we could have qb.ext.combined():

function combined(query, qtext, options) {
  if (_.isObject(qtext) && !options) {
    options = qtext;
    qtext = null;
  }

  return {
    search: {
      query: query.query || query,
      qtext: qtext,
      options: options.options || options
    }
  };
}

Is that what you mean?

[grtjn]

Something like that yes..

[joemfb]

I can add that here and reference it in the qb.text() deprecation message.

My plan:

• merge this
• take everything from your PR that doesn't break backwards compat, put in a new PR, merge that
• release (1.0.0 ?)
• add the other stuff we want, breaking backwards compat
• release (2.0.0 ?)

That second PR could include all your constraint stuff, container queries, etc. Everything except for collection/range etc. that conflicts with the deprecated methods.

Somewhere along the lines, we could extend the test suite to start comparing against the official client, as you recommended.

Thoughts?

[grtjn]

Let's start with testing this first. You think back-to-back test with old code could be possible? Kind of automated I mean?

[joemfb]

Can you give an example?

[grtjn]

Nothing too fancy, but something like expect(newQb.andQuery()).toEqual(oldQb.andQuery()). The old (current) one is pretty small so you could shove it entirely inside the test code. Just for the purpose of making explicit where overlap/mismatch is. Perhaps also check for deprecation messages somehow?

[joemfb]

The query-builder already had 100% test coverage, and I've added assertions comparing the deprecated functions to their replacements (which already pass all the original tests). Is there a specific scenario that would be covered by including an old copy of the query-builder?

[grtjn]

It is just that 100% coverage is not necesarily the same as 100% equality. We can get rid of it once proven..

[joemfb]

I agree that it's not the same, but I still don't understand the need for such testing. This PR is almost entirely reorganizing / renaming methods, with little to no implementation changes.

And, as an extra safeguard, there's also pretty extensive query-builder testing included in the searchContext in ml-search-ng (testing the relationship between the searchContext state and the constructed query).

Given all that, what are the kinds of incompatibility or regression you feel we need to guard against?

BTW, I think that style of testing will be important in the future, as we try to become more compatible with the official query-builder from the node-client-api, but that's an entirely different scenario.

[grtjn]

But.. this does change the implementation! The change couldn't be bigger! :-)

Call me paranoid if you like, but I just want proof that the and query from client-api produces the same as what the old qb would produce. Both return json right? Did you literally compare? Or maybe there are big differences. And that is not an issue. I just like to know about them. Back to back testing is the ideal way to reveal any difference.

By the way it is just a handful methods to compare. Like me to do it?

[joemfb]

I think we got our wires crossed. This PR doesn't introduce new methods, or new method implementations (except for combined()); it's just method-level refactoring of the old query-builder. It moves methods around, and then links the old method to the new method, logging a deprecation message. That's why I think the current testing is sufficient (it runs the old tests against the new method names, and additionally compares the output with the old method names).

This PR is only the first step in the plan I laid out above. The goal is to eventually be API compatible with a subset of the official node-client API; that's why all the constraint methods (plus operatorState() and combined()) are not on qb, but on qb.ext.

I was a little vague in my description beforehand, so let me describe the plan in more detail

• this PR set's up the skeleton of the API we want to have, while maintaining backwards compatibility with the previous version
• the next step is to add any additional methods we want available now, except for those that would break backwards compatibility with the previous version
• then, we do a release, so that anything depending on the query-builder can have a cycle to migrate off the deprecated methods. This won't break backwards compatibility, so the version number doesn't have to be a major increment (v0.0.6 -> v1.0.0); not sure what it should be
• next, we remove the deprecated methods, and add the additional methods we want to have (range(), collection(), etc.), breaking backwards compatibility. At this point, we'll be aiming for rough API compatibility with the node-client-api (at least as for as method names/signatures is concerned, I don't think we need to support all the use-cases)
• then, we do another release. This version number has to be a major increment (v1.0.0 -> v2.0.0)

After all those steps are completed, I think it'll make sense to start testing against the node-client-api, and making changes where the subset of methods here differ from their counterparts there.

Does this make sense? Do you still think that there's unacceptable risk?

[grtjn]

Oooooh! My bad, must have been blind! Doing too much in parallel I guess.. :-P

Yes, these steps make sense to me. The extra releases give people the option to stick with 0.x or 1.0 if they like. 1.0.0 sounds right for a qb that supports custom constraints, and geospatial. 2.0.0 is a good version number for the qb based on node-client-api.

[grtjn]

But I still wouldn't mind some comparison with node-client-api early on. I never worked with that, no experience at all..