Search param builder #509

patrickarlt · 2019-04-11T17:50:09Z

After some initial research in #384 (comment) I decided to implement a builder for search queries. Here is some basics of the API:

new SearchQueryBuilder()
  .match("test")
  .in("tags"); // "tags: test"

new SearchQueryBuilder()
  .startGroup()
    .match("test")
    .in("tags")
  .endGroup()
  .not()
  .match("seed")
  .in("tags"); // "(tags: test) NOT tags: seed"

const NOW = Date.now();
const FIVE_DAYS_AGO = new Date(Date.now() - (24*60*60*1000) * 5);

new SearchQueryBuilder()
.from(FIVE_DAYS_AGO)
.to(NOW)
.in("created")
.and()
.match("test")
.in("tags"); // "created: [1554570502723 TO 1555002502723] AND tags: test"

new SearchQueryBuilder()
.match("patrickarlt7104")
.in("owner")
.and()
.startGroup()
  .match("Web Mapping Application")
  .in("type")
  .or()
  .match("Mobile Application")
  .in("type")
  .or()
  .match("Application")
  .in("type")
.endGroup()
.and()
.match("VTS")
.in("*"); //owner: patrickarlt7104 AND (type: Web Mapping Application OR type: Mobile Application OR type: Application) AND fire

const query = new SearchQueryBuilder()
.match("patrickarlt7104")
.in("owner")
.and()
.startGroup()
  .match("Web Mapping Application")
  .in("type")
  .or()
  .match("Mobile Application")
  .in("type")
  .or()
  .match("Application")
  .in("type")
.endGroup(); // "owner: patrickarlt7104 AND (type: Web Mapping Application OR type: Mobile Application OR type: Application)"

query.clone()
  .and()
  .match("fire")
  .in("*"); // "owner: patrickarlt7104 AND (type: Web Mapping Application OR type: Mobile Application OR type: Application) AND fire"

Instead of trying to implement all sorts of crazy helpers like andHasTags and orHasTags ect... I sent for a simpler approach that matches what the REST API search query thing is. This simply tries to build the string as best as possible and should allow for almost ANY permutation of what can be passed to the search API.

In the end there is no way to type option 2 from #384 (comment) without a TON of extra work (it might not even be possible at all) so I went with a class based approach.

I think that eventually we will want both IParamBuilder for building a single param and IParamsBuilder for returning an object with multiple params. This PR limits itself to IParamBuilder for now and leaves IParamsBuilder open for things like routing or feature layer/webmap creation later. I think that for building complex JSON objects we could use something like https://medium.com/@bensammons/building-a-fluent-interface-with-typescript-using-generics-in-typescript-3-4d206f00dba5 would be 💯 but I'll play around with it later.

This PR is still in draft while:

We discuss the general API around IParamBuilder
We discuss the specific API of SearchQueryBuilder
I harden the API of SearchQueryBuilder to be more forgiving of stupid things like new SearchQueryBuilder().endGroup().and().or().from("foo").and().not("bar")
I write tests for SearchQueryBuilder
I write tests for IParamBuilder
Write more documentation around
1. Adding more builders for other params
2. A guide topic to using the SearchQueryBuilder
Replace the hellscape of https://github.com/Esri/arcgis-rest-js/blob/master/demos/node-cli-item-management/index.js#L102-L132 with the new builder

jgravois · 2019-04-11T21:16:12Z

cc/ @pranavkulkarni

tomwayson

Impressive @patrickarlt! I didn't look too deeply into the code, but at the macro level I like the direction you are headed.

Definitely wise to scope down to SearchQueryBuilder and IParamBuilder. The API looks good to me.

I'm cool w/ classes, and am thankful for all the thought you've put into the alternatives. The more "own special hells" we can avoid the better.

I'm especially cool w/ classes if we can move the builders into their own package as I suggest here: #137 (comment)

Finally, I assume the reason you brought withOptions() and withUrl() into this is just to that they play nicely w/ the builders, right? In general, I'd like to see those decoupled from the builder, and maybe even defaults (full disclosure, I haven't looked at #508 yet). I mean, it seems like they could be PR'd against master today, right?

packages/arcgis-rest-request/src/utils/process-params.ts

pranavkulkarni · 2019-04-12T19:52:41Z

This is awesome. I am currently porting some of the existing query building logic opendata-ui to hub.js/search. I could use this SearchQueryBuilder which is very intuitive and will help develop hub.js better. Any idea when this will roll out tentatively?

jgravois · 2019-04-12T21:15:18Z

Any idea when this will roll out tentatively?

our goal is to include this in a v2.0.0 release fairly quickly. my hope would be to ship by late next week.

if you could symlink pat's branch in the meantime and provide more detailed feedback, that would be really cool.

patrickarlt · 2019-04-12T21:22:06Z

@pranavkulkarni I would really want to land this in 2.0.0 next week.

@jgravois I'm probably going to be in Redlands sometime next week but I might only have 1-2 meetings do you and maybe @tomwayson want to spend a day on releasing 2.0.0?

jgravois · 2019-04-12T21:32:02Z

do you and maybe @tomwayson want to spend a day on releasing 2.0.0?

for. sure.

jgravois

i'd say rename it to toParam() and this is g2g!

it seems like they could be PR'd against master today, right?

it could, but i think we should release with v2.0.0. its a great 🥕.

patrickarlt · 2019-04-13T14:33:00Z

Since most people seem ok with the API I went ahead a wrote a pretty full test suite and added a bunch of warnings when people do stupid things like:

const query = new SearchQueryBuilder()
  .match("test")
  .not()
  .and()
  .toParam();

I would still like to write at least API doc for this. I'll do written doc some time after this merges. I also might replace IParamBuilder with something more like IOptionsBuilder and have it build out IRequestOptions. Then make a custom SearchOptions builder class just to demonstrate the other levels of builders. I'm just not 100% on board with making people leverage params.

patrickarlt · 2019-04-13T14:35:45Z

Since this will go into 2.0.0 I'm also going to handle #137 (comment).

patrickarlt · 2019-04-16T00:14:26Z

@jgravois @tomwayson OK I have done some major refactoring of searchItems. This is technically for 2.0.0 but I'm making the PR against master and @jgravois and I can figure it out from there.

Does the following:

Add support for a IParamBuilder which will allow for custom builder classes to provide the value of a single param in the API.
Concrete implementation of IParamBuilder in SearchQueryBuilder for the q param in search items.
Rewrite appendCustomParams and move it to common. appendCustomParams now accepts a custom IWhateverOptions object and a whitelist of keys to copy to params and returns the finished IRequestOptions object to use. You can optionally pass in a default set of options which will be merge before the keys are copied over.

This has 100% test coverage and should be ready to merge.

@jgravois feel free to refactor this and change the base to the 2.0.0 branch.

tomwayson

I'm happy to see searchForm go bye bye!

packages/arcgis-rest-request/src/utils/IParamsBuilder.ts

tomwayson · 2019-04-16T01:04:30Z

packages/arcgis-rest-common/package.json

@@ -13,7 +13,8 @@
    "dist/**"
  ],
  "dependencies": {
-    "tslib": "^1.9.3"
+    "tslib": "^1.9.3",
+    "@esri/arcgis-rest-request": "^1.19.2"


should this be a devDependency?

i'd say yes. fixed in 99300a8

tomwayson · 2019-04-16T01:17:42Z

packages/arcgis-rest-items/src/search.ts

@@ -29,6 +27,7 @@ export interface ISearchResult {
  num: number;
  nextStart: number;
  results: IItem[];
+  nextPage?: () => Promise<ISearchResult>;


This is clever. Should we also include a previousPage()?

gotta save something for 2.1!

codecov · 2019-04-16T04:42:44Z

Codecov Report

❗ No coverage uploaded for pull request base (v2.0.0@0f750ea). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             v2.0.0   #509   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?     93           
  Lines             ?   1317           
  Branches          ?    231           
=======================================
  Hits              ?   1317           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
packages/arcgis-rest-request/src/request.ts	`100% <ø> (ø)`
packages/arcgis-rest-request/src/index.ts	`100% <ø> (ø)`
packages/arcgis-rest-feature-service/src/delete.ts	`100% <100%> (ø)`
...es/arcgis-rest-request/src/utils/process-params.ts	`100% <100%> (ø)`
...es/arcgis-rest-request/test/mocks/param-builder.ts	`100% <100%> (ø)`
packages/arcgis-rest-common/src/index.ts	`100% <100%> (ø)`
packages/arcgis-rest-geocoder/src/geocode.ts	`100% <100%> (ø)`
...arcgis-rest-portal/src/items/SearchQueryBuilder.ts	`100% <100%> (ø)`
packages/arcgis-rest-portal/src/items/add.ts	`100% <100%> (ø)`
...es/arcgis-rest-feature-service/src/queryRelated.ts	`100% <100%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f750ea...3a56a23. Read the comment docs.

jgravois · 2019-04-16T04:48:57Z

i just pushed up two more commits with a handful of tweaks to get tests passing on CI and use @patrickarlt's new appendCustomParams method in feature-service and geocoder.

doing this helped me catch (and fix) a couple more edge cases in the refactored utility method.

if @tomwayson is happy, i can rebase this branch on top of v2.0.0 and resolve the conflicts.

patrickarlt · 2019-04-16T14:07:05Z

@jgravois your commits LGTM. I'll let you refactor this into 2.0.0.

…m-builder-rebased

patrickarlt added enhancement needs review labels Apr 11, 2019

patrickarlt requested review from dbouwman, tomwayson and jgravois April 11, 2019 17:50

patrickarlt self-assigned this Apr 11, 2019

tomwayson reviewed Apr 12, 2019

View reviewed changes

packages/arcgis-rest-request/src/utils/process-params.ts Outdated Show resolved Hide resolved

jgravois mentioned this pull request Apr 12, 2019

search package Esri/hub.js#152

Merged

inital prototype of param builder

c3dd95a

patrickarlt force-pushed the search-param-builder branch from 7b6204d to c3dd95a Compare April 12, 2019 21:30

jgravois approved these changes Apr 12, 2019

View reviewed changes

patrickarlt added 2 commits April 13, 2019 07:27

passing tests for SearchQueryBuilder

7bf9fa3

passing tests for SearchQueryBuilder, IParamBuilder and IParamsBuilder

2b040fb

patrickarlt mentioned this pull request Apr 13, 2019

Draft: composition and defaults #508

Merged

patrickarlt force-pushed the composition-and-defaults branch from d89be63 to 38b3500 Compare April 15, 2019 14:33

patrickarlt added 2 commits April 15, 2019 12:31

export SearchQueryBuilder

8091c02

refactor searchItems to drop searchForm option

aaa2cbc

patrickarlt changed the base branch from composition-and-defaults to v2.0.0 April 15, 2019 23:09

patrickarlt marked this pull request as ready for review April 15, 2019 23:11

refactor searchItems to drop searchForm option

f9225db

patrickarlt changed the base branch from v2.0.0 to master April 15, 2019 23:13

remove params builder support

eb297cb

tomwayson reviewed Apr 16, 2019

View reviewed changes

use new appendCustomParams consistently

99300a8

more tests

82fddd5

jgravois added 3 commits April 16, 2019 08:41

tslib isnt a peerDep

63a326e

Merge remote-tracking branch 'upstream/v2.0.0' into chore/search-para…

5ed5de1

…m-builder-rebased

readd auth as peerDep of feature-service

3a56a23

jgravois changed the base branch from master to v2.0.0 April 16, 2019 16:22

jgravois merged commit 3f2d786 into v2.0.0 Apr 16, 2019

jgravois deleted the search-param-builder branch April 16, 2019 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search param builder #509

Search param builder #509

patrickarlt commented Apr 11, 2019 •

edited

Loading

jgravois commented Apr 11, 2019

tomwayson left a comment

pranavkulkarni commented Apr 12, 2019

jgravois commented Apr 12, 2019 •

edited

Loading

patrickarlt commented Apr 12, 2019

jgravois commented Apr 12, 2019

jgravois left a comment •

edited

Loading

patrickarlt commented Apr 13, 2019

patrickarlt commented Apr 13, 2019

patrickarlt commented Apr 16, 2019

tomwayson left a comment

tomwayson Apr 16, 2019

jgravois Apr 16, 2019

tomwayson Apr 16, 2019

jgravois Apr 16, 2019

codecov bot commented Apr 16, 2019 •

edited

Loading

jgravois commented Apr 16, 2019

patrickarlt commented Apr 16, 2019

Search param builder #509

Search param builder #509

Conversation

patrickarlt commented Apr 11, 2019 • edited Loading

jgravois commented Apr 11, 2019

tomwayson left a comment

Choose a reason for hiding this comment

pranavkulkarni commented Apr 12, 2019

jgravois commented Apr 12, 2019 • edited Loading

patrickarlt commented Apr 12, 2019

jgravois commented Apr 12, 2019

jgravois left a comment • edited Loading

Choose a reason for hiding this comment

patrickarlt commented Apr 13, 2019

patrickarlt commented Apr 13, 2019

patrickarlt commented Apr 16, 2019

tomwayson left a comment

Choose a reason for hiding this comment

tomwayson Apr 16, 2019

Choose a reason for hiding this comment

jgravois Apr 16, 2019

Choose a reason for hiding this comment

tomwayson Apr 16, 2019

Choose a reason for hiding this comment

jgravois Apr 16, 2019

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2019 • edited Loading

Codecov Report

jgravois commented Apr 16, 2019

patrickarlt commented Apr 16, 2019

patrickarlt commented Apr 11, 2019 •

edited

Loading

jgravois commented Apr 12, 2019 •

edited

Loading

jgravois left a comment •

edited

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading