[#3] Reduce invalid document type severity to warning message

[jpan127]

First time really using Javascript and first time making a PR to an open source project. Please let me know if anything is incorrect. Thanks!

[missinglink]

I think changing module.exports.truthy to warn instead of throw is not the correct solution here.

The function module.exports.truthy is used by several setters and so changing the behavior in this function will have an effect on all setters which use it.

The function throws an error in order to prevent invalid data from entering the database.

If there is an issue with a specific setter function then the issue would be better addressed there.

Sorry, we can't merge this as-is because it would have a major impact on existing users and scripts which depend on the existing behaviour.

[missinglink]

Was your intention to allow invalid ids?

This would be a major change in validation logic which I don't think we could merge.

[orangejulius]

This is connected to pelias/polylines#3.

It turns out I was wrong @jpan127, I didn't realize that the exception being thrown was thrown from within pelias/model. I agree with @missinglink that we can't change that here. Sorry about that.

Instead I think the approach we'd want to take is to add some validation in the polylines importer itself, to avoid creating invalid Document objects in the first place. The relevant line that seems to throw errors is https://github.com/pelias/polylines/blob/master/stream/document.js#L31

[jpan127]

Ah I see, that does make sense, did not realize it would be such an impacting change. Thanks everyone for the feedback.

@orangejulius Am I understanding you right? I was thinking you mean something like this:

Document.js

Document.prototype.setName = function( prop, value ){

  validate.type('string', value);
  // validate.truthy(value);

  // must copy name to 'phrase' index
  if( Array.isArray( this.name[ prop ] ) ){
    this.name[ prop ][ 0 ] = value;
    this.phrase[ prop ][ 0 ] = value;
  } else {
    this.name[ prop ] = value;
    this.phrase[ prop ] = value;
  }

  return this;
};

document.js

  return through.obj( function( geojson, _, next ){
    const name = geojson.properties.name;
    if( (typeof name === 'string' && !name.trim() ) || !name ) {
      return;
    }

    try {

      // uniq id
      var id = [ idprefix, uid++ ].join(':');

      // create new instance of pelias/model
      var doc = new model.Document( source, layer, id );

      // name
      doc.setName( 'default', name );

      // street
      doc.setAddress( 'street', name );

      // centroid
      var prop = geojson.properties.centroid;
      doc.setCentroid({ lon: prop[0], lat: prop[1] });

      // distance meta data (in meters)
      doc.setMeta( 'distance', geojson.properties.distance );

      // bounding box
      doc.setBoundingBox({
        upperLeft: {
          lat: geojson.bbox[3],
          lon: geojson.bbox[0]
        },
        lowerRight: {
          lat: geojson.bbox[1],
          lon: geojson.bbox[2]
        }
      });

      // push downstream
      this.push( doc );

    } catch( e ){
      logger.error( 'polyline document error', e );
    }

    next();
  });
}

[orangejulius]

Hey @jpan127,
Sorry, this dropped off my radar, and I didn't realize your last message contained a proposed change. But yes, what you're doing at the top of stream/document.js in the polylines importer looks like the right track.

To be clear, changes should be contained to the polylines importer. I think pelias/model will have to stay how it is. I'm going to close this PR but look forward to seeing if you find something that works well in the polylines repo!