koopjs / winnow

Deprecated
Apache License 2.0
90 stars 18 forks source link

Normalize options not working when inSR is JSON string #81

Closed jkerr5 closed 6 years ago

jkerr5 commented 6 years ago

Insights 2.3 is passing a query using URLs that look like this

https://<featurelayer-url>/query?f=json&geometry=%7B%22xmin%22%3A-20037508.342788905%2C%22ymin%22%3A-20037508.342788905%2C%22xmax%22%3A0%2C%22ymax%22%3A0%7D&inSR=%7B%22wkid%22%3A102100%7D&geometryType=esriGeometryEnvelope&spatialRel=esriSpatialRelIntersects&outFields=url%2COBJECTID&returnGeometry=true&outSR=%7B%22wkid%22%3A102100%7D&resultOffset=0&where=1%3D1&callback=dojo.io.script.jsonp_dojoIoScript12._jsonpCallback

The inSR is a JSON string and is not being parsed into a JSON object so is not being normalized correctly.

Is this an issue with the way Insights is passing in the Spatial reference JSON object or should the string be parsed into a JSON object by winnow?

rgwozdz commented 6 years ago

@jkerr5 just to make sure, are you using Winnow version 1.15.3 (most recent, just released a few days ago)? That release included a fix for inSR arriving as JSON. Just want to make sure, because when I test your query string locally, that Winnow 1.15.3 seems to handle it without issue. I'll dig deeper if the problem occurs for you using 1.15.3.

jkerr5 commented 6 years ago

I am running 1.15.3. Can you point to where the fix is in winnow? I don't see how it could work given the code that's there in 1.15.3.

To be clear though, I am calling the the options prepare() function directly in my provider as I need it to convert the query geometry into WGS84. Is there some other code path that handles the conversion of parameters that are JSON strings into objects?

I am actually parsing the JSON for a number of parameters in my provider due to this issue already but the inSR one is new (I guess maybe they changed it in Insights 2.3). You can see my code here: https://github.com/koopjs/koop-provider-marklogic/blob/master/src/koop/marklogic.js#L20

rgwozdz commented 6 years ago

https://github.com/koopjs/winnow/blob/master/src/options/normalizeOptions.js#L82-L108.

For me, when I have a request with inSR=%7B%22wkid%22%3A102100%7D as you pasted above, options.inSR arrives in that function with a value of {wkid: 102100}. I believe it's parsed from string to object earlier on, here. Since is arrives as {wkid: 102100}, the wkid value is extracted here, and then it is returned as 'EPSG:3857'. At least that's what I observe when I step thru with the debugger. I don't have your provider though.

rgwozdz commented 6 years ago

@jkerr5 just circling back on this after some additional thought. The function expects that any JSON strings have already been parsed into objects. I think this makes sense; we should probably do all that parsing prior to winnow normalize functions, else we might have a lot of repeated code for parsing scattered among the functions. As noted above, FeatureServer does this parsing, but if you are using winnow outside of FeatureServer, you would have to implement something similar.

jkerr5 commented 6 years ago

In my experience, the JSON strings in the URL parameters are not parsed to objects before the provider's getData() function is called. Maybe I am doing something wrong in the provider?

rgwozdz commented 6 years ago

sorry @jkerr5, I don't think you are doing anything wrong. I'm just not articulating myself very well. I'll try to explain where the parsing is happening in a general way when using Koop/koop-output-geoservices/FeatureServer/Winnow.

1) request received by koop. If it's a FeatureServer route, koop-output-geoservice gets the data from the provider and passes this data, as well as the Express request and response object, on to the FeatureServer module.

2) In FeatureServer, an attempt is made to parse all req.query parameters to JSON objects. https://github.com/koopjs/FeatureServer/blob/master/src/route.js#L30-L32. After the req.query parameters should be strings, numbers or JSON objects.

3) FeatureServer passes the parsed req.query parameters on to winnow https://github.com/koopjs/FeatureServer/blob/master/src/query.js#L18-L33

So, if your req.query parameters arrive in Winnow's normalize functions via FeatureServer, they should already be parsed from JSON strings into objects.

rgwozdz commented 6 years ago

If they are still getting into Winnow unparsed, can you confirm for me that the request is coming through FeatureServer before going to Winnow? If it's still unparsed something unexpected (at least to me :) ) is happening.

jkerr5 commented 6 years ago

Right, so the provider's getData() function is called in step 1 in your flow so none of the parameters would be parsed into JSON objects yet. This means that if the provider needs to do anything with those parameters, it needs to parse them too.

My provider is trying to leverage the winnow functions to change the incoming geometry query into WGS84 so it can be passed thru to the database query. Because the parameters are not parsed until after the provider is called, that logic appears to have to be done in two places.

Is there a reason, koop-output-geoservice wouldn't do that prior to calling the provider?

rgwozdz commented 6 years ago

Ok, now I understand. I think your question is a good one. I think the general idea with koop-output-geoservices has been to keep it as simple as possible, and not do much processing there - it's just a wrapper/router for FeatureServer. But I do see benefits of moving the parsing of query parameters out of FeatureServer. Perhaps the parsing of req.query JSON strings should actually be in koop-core. @dmfenton do you have thoughts on this?

dmfenton commented 6 years ago

Is there a situation where parsing a JSON string into an object would be undesirable? I think parsing JSON strings in core makes sense even thought not all output APIs will deal with query parameters in the same way.

rgwozdz commented 6 years ago

The only situation that comes to mind is if the incoming JSON string needed to be persisted as a string type to a data-store - something that didn't allow for object storage for example. That's easily remedied by re-stringifying it. So, I guess, if there is someone out there doing that, moving the parse into koop-core would be a breaking change.

dmfenton commented 6 years ago

I'd say that's probably not a good enough reason not to parse. Anything writing to a datastore should be responsible for it's own serialization format. I also don't know of any outputs that exist today with this problem. I think we can go ahead and make the change.

rgwozdz commented 6 years ago

Sounds good. I have opened a koop-core issue here.

rgwozdz commented 6 years ago

@jkerr5 we have added JSON parsing of query params to koop-core in v3.8.0. Thanks for your input on this!

jkerr5 commented 6 years ago

Thanks!