joemfb / ml-common-ng

An angular module of common components for the MarkLogic REST API
https://joemfb.github.io/ml-common-ng/
2 stars 2 forks source link

add backward-compatible query-builder methods #14

Closed joemfb closed 9 years ago

joemfb commented 9 years ago

Part of the query-builder improvement plan documented here: https://github.com/joemfb/ml-common-ng/pull/13#issuecomment-139840486.

re #1

Add methods to the query-builder (and the query-builder ext property) that don't conflict with existing deprecated methods.

examples (from #11)

For each method, add it directly to qb if it exists in the node-client-api query-builder, making sure to match the name and signature. Otherwise, add it to qb.ext. Additionally, if it's a constraint method, add it to qb.ext.constraint().

joemfb commented 9 years ago

I've left out geospatial for now. If we're gonna strive for compatibility, we'll have to implement circle(), region(), point(), latlon(), box(), etc. Lots of edge cases to test ...

geospatial is probably the area where we'd get the most benefit from just using the official api.

/cc @grtjn

grtjn commented 9 years ago

Geospatial is also the one I am waiting for the most, particularly the custom geospatial. Alas.. :P

The custom-geospatial is a bit of a special case, not particularly well documented. It goes back to earliest times of app-builder. It is essentially a custom constraint, but with box/circle/polygon property instead of the typical values property if I am not mistaken. So, not particularly difficult to implement, but to use the improved app-builder custom geo facet code (used by Vanguard for all kinds of demos), support for custom constraints, and particularly the custom geospatial one will be crucial..

joemfb commented 9 years ago

We can come up with something if we have to have it; it just won't be forward-compatible.

Can you point me towards an implementation of this custom-geospatial constraint?

grtjn commented 9 years ago

You mean this? https://github.com/grtjn/ml-common-ng/blob/1-improve-querybuilder/src/ml-query-builder.service.js#L234

Or looking for the xqy code?

joemfb commented 9 years ago

The XQY implementation.

grtjn commented 9 years ago

You should be able to find the original app-builder one in your local ML installation under Assets\appbuilder\components\appbuilder-2.0\constraints\geo.xqy. You would configure it with for instance:

  <constraint name="Home">
    <custom>
      <parse apply="parse-structured" ns="http://marklogic.com/appservices/viz/geo" at="/constraint/geo.xqy"/>
      <start-facet apply="start" ns="http://marklogic.com/appservices/viz/geo" at="/constraint/geo.xqy"/>
      <finish-facet apply="finish" ns="http://marklogic.com/appservices/viz/geo" at="/constraint/geo.xqy"/>
    </custom>
    <annotation>
      <geo-attr-pair>
        <heatmap n="53.610165" e="9.0719604" s="50.921799" w="1.7330933" latdivs="3" londivs="6" />
        <parent ns="" name="a"/>
        <lat ns="" name="home-lat"/>
        <lon ns="" name="home-lng"/>
      </geo-attr-pair>
    </annotation>
  </constraint>
joemfb commented 9 years ago

No PR yet, since it'd depends on #16 and make an ugly diff, but here's a first crack at geospatial:

https://github.com/joemfb/ml-common-ng/tree/geospatial

Commit here: https://github.com/joemfb/ml-common-ng/commit/f42c7d64808a2de82b32129c17019a73a70cb195

grtjn commented 9 years ago

I don't think it is necessary to check for Array of objects. The logic would be something like, if it is an object, then we assume it is something like { shapes: [...] }, which will contain arrays itself. The passing-in-as-object is just a convenient way to allow custom property names. I don't see immediate use for an array of such items. If you like to add multiple properties, you can add them in that one object. Maybe that simplifies things a bit.

So, basically: an atomic value will be wrapped as before in an array, and added as a text or value property. An array is passed through directly as text/value property. And an object gets merged in a straight-forward way..

Or something along those lines.. (no need to make it hard for ourselves.. ;))

joemfb commented 9 years ago

Here's what I pictured:

qb.ext.customConstraint('my-geo', [
  qb.ext.geospatialValues(
    {  latitude: 1, longitude: 2 },
    { south: 1, west: 2, north: 3, east: 4
  ),
  { annotation: 'blah' }
])

In other words, if you're constructing your values object through some other method, you'd have to bind to an intermediate variable, than set additional properties on it. This approach gives some more flexibility. Can't say how often it'd be used, but it doesn't make the implementation much more complex.

Maybe overthinking, but it's written and works ... ;)

grtjn commented 9 years ago

Yeah, kinda makes sense like this. I'd be fine with this..

joemfb commented 9 years ago

All relevant PRs merged into master