Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tasks.IdentifyFeatures not returning GeoJson with ID #686

Closed
ronoel opened this issue Dec 4, 2015 · 12 comments · Fixed by #848
Closed

Tasks.IdentifyFeatures not returning GeoJson with ID #686

ronoel opened this issue Dec 4, 2015 · 12 comments · Fixed by #848
Assignees
Labels

Comments

@ronoel
Copy link

ronoel commented Dec 4, 2015

The callback come GeoJSON without ID. But GeoJSON has all properties including objectid.

@jgravois
Copy link
Contributor

jgravois commented Dec 4, 2015

right now we have a little logic written into responseToFeatureCollection to parse ArcGIS Server responses looking for metadata about the layer to glean the name of the id field. for query requests, this works great.

but since identify requests don't pass back metadata (because features from more than one layer can be retrieved), we're currently just hoping the fieldname is "OBJECTID".

we could throw in a few more checks that interrogate the first feature for other common casing and fix the problem 80% of the time, but its just a hack because there's no guarantee that the next feature is even coming from the same layer.

else if (response.results && response.results.length > 0) {
  // dig into the first result to see if 'objectid' (all lowercase) is present  
  if (response.results[0].attributes.objectid) {
      objectIdField = 'objectid';
  // etc.  
  } else if (response.results[0].attributes.ObjectID) {
      objectIdField = 'ObjectID';
    }

  } else {
    objectIdField = 'OBJECTID';
  }

@patrickarlt may have a more clever idea, but it doesn't seem to me that its worth adding more checking than the above to this hot code.

alternatively perhaps we could give developers an opportunity to tell us the fieldname they'd like to use when you set up the task (best revisited when looking more deeply at #633)

edit: i'll also play around with trying to implement a case insensitive regex.

@patrickarlt
Copy link
Contributor

@jgravois case insensitive regex sounds like a good bet. That should cover most known cases. Assigning to you.

@theashyster
Copy link

Hi, with the case insensitive regex in place the GeoJSON is now indeed returned together with an ID, but this ID is a String for Tasks.IdentifyFeatures compared to Tasks.Query that returns a GeoJSON with an ID that is an Integer.

Could this be fixed to be consistent across Tasks?

@jgravois jgravois reopened this Oct 21, 2016
@jgravois
Copy link
Contributor

Could this be fixed to be consistent across Tasks?

it can, and absolutely should. thanks for reporting.

@jgravois
Copy link
Contributor

jgravois commented Nov 9, 2016

i finally got a chance to sit down and look at this today and was surprised to find that our developers made a conscious decision to pre-format the attributes returned by /identify and /find (but not /query).

example query response:

"features": [{
  "attributes": {
    "OBJECTID": 7,
    "POP2000": 493782,
    "POP2007": 523174
  }
}],

example identify response:

"results": [{
  "attributes": {
    "OBJECTID": "7",
    "Shape": "Polygon",
    "POP2000": "493782",
    "POP2007": "523174"
  }
}]

we could cast the objectIdField value to an int manually, but this would have the undesired impact of mutating the raw response too.

// within Util.responseToFeatureCollection
if (count) {
  if (typeof features[0].attributes[objectIdField] === 'string') {
    var needsCast = true;
  }
  for (var i = features.length - 1; i >= 0; i--) {
    if (needsCast) {
      features[i].attributes[objectIdField] = +features[i].attributes[objectIdField];
    }
    var feature = arcgisToGeoJSON(features[i], objectIdField);
    featureCollection.features.push(feature);
  }
}

is the discrepancy in the results really problematic for you? if so, how?
i'm willing to discuss it further but my hunch is that its not worth shoehorning a lot of extra logic into this hot code unless its absolutely necessary.

@theashyster
Copy link

@jgravois Ok, thanks for finding the time to look at this more closely.

What I find odd is - why they decided to pre-format /identify and /find, but not /query then?

I see and understand that you don't want to mess with the raw response and can understand that.

It is not really problematic, we already handle this in our code, but it would be nice if all the services that return the same kind of data, like in your example OBJECTID, POP2000, STATE_NAME returned the data in the same format across these services.

Probably you are right, that it is not worth adding this extra logic if it just creates extra complexity and does not give any real value to the library.

Just for the record, in our case we take the properties of each feature in the feature collection returned from /identify and run it through a function that extracts the fields we need. When using /query it is possible to specify .fields() and only those will be returned, but for /identify we sanitize the returned data, so they look exactly the same as the ones returned by /query. We do this because we display the returned data from /query on a map and then add the data returned from /identify to the same layer on the map when user clicks something on the map. So we keep both data (features from /query and /identify) in the same layer and they cannot differ in the information they contain.

So at the end when I receive data from /identify I have to go through each feature from the feature collection and do the sanitizing of the feature properties and after that I have to do this:

featureCollection.features[featureIndex].id = parseInt(featureCollection.features[featureIndex].id, 10);
delete featureCollection.features[featureIndex].layerId;

@jgravois
Copy link
Contributor

thanks for taking the time to explain your workflow.

why they decided to pre-format /identify and /find, but not /query then?

i'm sure they have their reasons, but i'm definitely in your camp. i would prefer consistency across operations too.

i'm going to go ahead and re-close the issue here, but if anyone wants to discuss the issue further in the future, i'd be happy to.

@theashyster
Copy link

@jgravois No problem, :)

i'm sure they have their reasons, but i'm definitely in your camp. i would prefer consistency across operations too.

Maybe this can be raised to them and they can think about having a more consistent responses across operations? Is there a project where I can raise it as a bug?

Yeah, no problem, thanks for your responses and time looking into this.

@jgravois
Copy link
Contributor

unfortunately its already been raised as a bug (several times), and rejected as designed:

MapServer REST Identify and Find operations should allow users to get unformatted raw data and field names in the results.
... United States - (en-US). SalesForce Defect: NIM084070. Planned Fix
Version: Programming Language: Server Platform: 7 ...
SalesForce Defect: NIM084070 | Status: Closed

Dates returned by the identify task in rest services directory do not contain the UTC string appended to them
... Beta: No. Additional Info: Duplicate of NIM084070. REASON FOR REJECTION.
Duplicate WORKAROUND. No workaround currently documented. ...
SalesForce Defect: NIM068802 | Status: Closed

REST Identify Task returns results where fields that are numeric in the underlying map have been converted to strings
... Customer Email: [email protected]. Beta: No. Additional Info:
Duplicate of NIM084070. REASON FOR REJECTION. Duplicate ...
SalesForce Defect: NIM086220 | Status: Closed

Using Identify on the REST endpoint of a European OS returns a string value for double and float attribute fields with a comma decimal separator while Query at the REST endpoint returns standard JSON numbers (period as the decimal separator). This causes an error when the results of one are used as input to the other.
... Related to NIM084070, except for double and string fields, not just date
fields. This issue does occur with file geodatabases ...
SalesForce Defect: BUG-000086125 | Status: Closed

@theashyster
Copy link

I see, sad to see that, but will have to live with it. Thanks, for the further information.

@jgravois
Copy link
Contributor

jgravois commented Jun 2, 2017

looks like someone else felt the same way as @theashyster and i.

in ArcGIS 10.5 a new parameter called returnUnformattedValues was added to both identify and find.

@theashyster
Copy link

@jgravois This is nice, they found a way for this to work both ways :) Now another fantastic option would be to be able to pass in outFields to Identify service the same way as we can do with Query service right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants