strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Support for `integer` datatype removed from loopback\strong-remoting #317

Closed jzogg closed 8 years ago

jzogg commented 8 years ago

IBM support sent me this way. I had created IBM service request 46011999000 because this is for GoDaddy and we have a paid subscription.

loopback version 2.18.1 strong-remoting version 2.19.0

converting parameter to target type includes explicit support for 'integer' in shared-method.js lines 390-443

function convertToBasicRemotingType(type) {
  if (Array.isArray(type)) {
    return type.map(convertToBasicRemotingType);
  }

  if (typeof type === 'object') {
    type = type.modelName || type.name;
  }

  type = String(type).toLowerCase();

  switch (type) {
    case 'string':
    case 'number':
    case 'date':
    case 'boolean':
    case 'buffer':
    case 'object':
    case 'any':
      return type;
    case 'integer':
      return 'number';
    case 'array':
      return ['any'].map(convertToBasicRemotingType);
    default:
      // custom types like MyModel
      return 'object';
  }
}

function convertValueToTargetType(argName, value, targetType) {
  switch (targetType) {
    case 'string':
      return String(value).valueOf();
    case 'date':
      return new Date(value);
    case 'number':
    case 'integer':
      return Number(value).valueOf();
    case 'boolean':
      return Boolean(value).valueOf();
    // Other types such as 'object', 'array',
    // ModelClass, ['string'], or [ModelClass]
    default:
      switch (typeof value) {
        case 'string':
          return JSON.parse(value);
        case 'object':
          return value;
        default:
          throw new badArgumentError(argName + ' must be ' + targetType);
      }
  }
}

loopback version 2.28.0 strong-remoting version 2.26.0

converting parameter to target type does not include support for 'integer' in shared-method.js lines 373-424

function convertToBasicRemotingType(type) {
  if (Array.isArray(type)) {
    return type.map(convertToBasicRemotingType);
  }

  if (typeof type === 'object') {
    type = type.modelName || type.name;
  }

  type = String(type).toLowerCase();

  switch (type) {
    case 'string':
    case 'number':
    case 'date':
    case 'boolean':
    case 'buffer':
    case 'object':
    case 'file':
    case 'any':
      return type;
    case 'array':
      return ['any'].map(convertToBasicRemotingType);
    default:
      // custom types like MyModel
      return 'object';
  }
}

function convertValueToTargetType(argName, value, targetType) {
  switch (targetType) {
    case 'string':
      return String(value).valueOf();
    case 'date':
      return new Date(value);
    case 'number':
      return Number(value).valueOf();
    case 'boolean':
      return Boolean(value).valueOf();
    // Other types such as 'object', 'array',
    // ModelClass, ['string'], or [ModelClass]
    default:
      switch (typeof value) {
        case 'string':
          return JSON.parse(value);
        case 'object':
          return value;
        default:
          throw new badArgumentError(argName + ' must be ' + targetType);
      }
  }
}

We have Swagger specs that have been using integer which is completely valid but a newer version of loopback we'd be forced to switch to number and manually handle checking for fractional values. I don't think that's appropriate.

raymondfeng commented 8 years ago

@jzogg What is your root requirement? To enable type: 'integter' for strong-remoting declarations? (We only support type: 'number' today).

raymondfeng commented 8 years ago

FYI: LoopBack swagger spec generation allows integer - https://github.com/strongloop/loopback-swagger/blob/master/lib/specgen/schema-builder.js#L13

gunjpan commented 8 years ago

After a discussion with Raymond & Ritchie, following is the proposal re. supporting integer data type: Enable type: integer with a strict flag such that:

@jzogg : Does that supports your use case. If not, could you please share your requirement.

cc/ @bajtos @ritch @raymondfeng If there is a consent for the above proposal, I'll update the PR#323 to reflect it.

bajtos commented 8 years ago

IMO, we should implement only strict:true behaviour right now (no flag involved) and wait until there is a clear need for strict:false before implementing that other mode.

gunjpan commented 8 years ago

@jzogg : FYI: the architects agreed on @bajtos's above proposal to implement only strict:true behaviour right now (no flag involved) and it'll be reflected in the patch.

gunjpan commented 8 years ago

Patch is landed, hasn't been released to npmjs yet. Will udpate when released (which would be ASAP)

bajtos commented 8 years ago

Released in strong-remoting@2.29.0.