koopjs / FeatureServer

An open source Geoservices Implementation (deprecated)
https://geoservices.github.io
Other
101 stars 32 forks source link

Handle empty query parameter values #252

Closed despainm closed 1 year ago

despainm commented 1 year ago

Some of our arcgis/esri clients provide requests that contain query parameters with empty values. We've noticed that two parameters in particular cause errors for our clients.

resultRecordCount=&groupByFieldsForStatistics=

When resultRecordCount= is provided the route.js query param validation does not handle empty/null values which causes a 400 to be returned. This is a reasonable response given that the arcgis documentation says The minimum value entered for this parameter cannot be below 1 and one can assume that an empty value does not meet this requirement.

When groupByFieldsForStatistics= is provided the filter-and-transform.js query call fails with Cannot convert undefined or null to object.

Is it possible to treat the empty values of these two parameters as if they weren't provided at all?

One suggestion is to update the route.js validateAndCoerceQueryParams to delete these fields from the queryParams if the values are null/empty.

function validateAndCoerceQueryParams (queryParams, { maxRecordCount }) {
  if (!queryParams.groupByFieldsForStatistics) {
    delete queryParams.groupByFieldsForStatistics
  }
  if (!queryParams.resultRecordCount) {
    delete queryParams.resultRecordCount
  }
  const { error, value: query } = queryParamSchema.validate(queryParams)

...
}
rgwozdz commented 1 year ago

Hi @despainm . Thank you for reaching out. Note the this FeatureServer repo is deprecated, and we moved the code abase to the Koop monorepo at https://github.com/koopjs/koop, specifically the package https://github.com/koopjs/koop/tree/master/packages/featureserver. All changes and new features should be made there. If you could move this issue over to that repo, that would be a help.

We should be able to make the changes you need. My preference would probably be to change the validator so that empty strings are valid, but then remove the empty string in followup normalization step. That way our validation schema aligns with all acceptable received values, rather than having us mutating responses in order to fit a validation schema we already control. If you could start a PR for this, that would probably expedite development. Thanks!

despainm commented 1 year ago

Thanks for the response!

Here is the issue against the koop project: https://github.com/koopjs/koop/issues/441

I'll work on starting a PR with your recommendations.

despainm commented 1 year ago

Replaced with https://github.com/koopjs/koop/issues/441