new validateObjectParam treats JSON null as undefined

[vnavascues]

Hi, I'm very pleased to see this new validation system.

I'd like to know the reasoning behind a null param is defaulted to undefined.

https://github.com/smartcontractkit/external-adapters-js/blob/develop/packages/core/bootstrap/src/lib/external-adapter/validator.ts#L218

    const param = usedKey
      ? this.input.data[usedKey as string] ?? inputConfig.default
      : inputConfig.default

    const paramIsDefined = !(param === undefined || param === null || param === '')

From my point of view null is a genuine JSON data type that can be required by an API in the request payload (POST/PUT request using input params). It can also be hard coded in the requestData (JSON) of a bridge task.

On another note, would you see any benefit at all creating an Enum (or similar) for the validation type (i.e. boolean, number, bigint, string, array, object) that can be exported elsewhere so it would be easier to see them all and less error-prone typing them?

Thanks!

[austinborn]

Hello, thank you for the feedback! I drove development on this system so I can share my perspective at least. In my experience, passing null as a value implies that the value is unnecessary and can be treated similarly to undefined. Do you have any examples of APIs that interpret null in a particular way?

For thetype validation, I assume you mean like this:

export enum Type {
  Array,
  BigInt,
  Boolean,
  Number,
  Object,
  String
}

And then it would be used as type: Type.String inside the inputParameter object?

[vnavascues]

Hello, thank you for the feedback! I drove development on this system so I can share my perspective at least. In my experience, passing null as a value implies that the value is unnecessary and can be treated similarly to undefined. Do you have any examples of APIs that interpret null in a particular way?

For thetype validation, I assume you mean like this:

export enum Type {
  Array,
  BigInt,
  Boolean,
  Number,
  Object,
  String
}

And then it would be used as type: Type.String inside the inputParameter object?

Thanks for this quick answer and I get your point of view. I don't have any particular API that falls into this case (let's be honest it is a rare one). I was just wondering if I ever had to POST a null property and I wanted to be able to explicitly receive it via input (no custom logic).

Regarding the validation, yes, but even something simpler like this (you can still use the typeof):

export enum InputParamType {
  ARRAY = 'array',
  BOOLEAN = 'boolean',
  BIGINT = 'bigint',
  NUMBER = 'number',
  OBJECT = 'object',
  STRING = 'string',
}

[vnavascues]

Hey @austinborn it's me again :)
A quick one (I don' want to open a new issue). I've keep testing the new inputParameters object notation on 1.10.0 and few things:

1. I can't trigger this exception with a wrong type

export const inputParameters: InputParameters = {
  id: {
    required: true,
    type: 'number', // tested with 'string' too, also with 'aliases'
  },
}

Requesting with a different type:

curl -X POST -H "content-type:application/json" "http://localhost:8080" --data '{ "id": 0, "data": {"id": "1"}}'

it returns:

{"jobRunID":0,"status":"errored","statusCode":400,"error":{"name":"AdapterError","message":"Required parameter id must be non-null and non-empty","feedID":"{\"data\":{\"id\":\"1\",\"resultPath\":\"redacted\",\"endpoint\":\"redacted\"}}"}}

2. Why do not allow [] and {} (both empty) as a default? I think it limits this feature potential. For instance here I'm interested in defaulting to {}.

I'm very much interested into adopting this new object notation and removing many custom validations implemented on previous EAs

Thanks again

[austinborn]

@vnavascues Are these projects you would be interested in taking on? I see a few separate issues that you're welcome to implement. I won't be able to work on these right now, but I'll make a note about them as well:

1. Enum for type attribute
2. Allow null, [] and {} as valid inputs
3. Ensure correct error messaging for incorrect input param types

[austinborn]

The team is receptive to allowing null, [] and {} as inputs, we just can't prioritize it right now since there isn't a definite use case.

[vnavascues]

@austinborn definitely I'm interested in these projects. I'll try to allocate at least 1 & 3 on my next sprint, and 2 on another. Thanks for looking at it and dedicating time to discuss it with your team.

[austinborn]

@vnavascues It looks like #1579 may resolve issue 3 from the list. Hope you didn't sink too much time into that one.

[vnavascues]

@vnavascues It looks like #1579 may resolve issue 3 from the list. Hope you didn't sink too much time into that one.

Thank you for this quick fix, works like a charm.

The only thing I'm not sure is the new case-insensitive options. Query parameters are case sensitive and with this change consumers don't have to be 100% accurate (as per README.md instructions) which means EA developers have to deal with it.
I understand though that this change makes much more sense from a price-feed-oriented-framework point of view (i.e. base and quote asset id/ticker).

[austinborn]

The only thing I'm not sure is the new case-insensitive options. Query parameters are case sensitive and with this change consumers don't have to be 100% accurate (as per README.md instructions) which means EA developers have to deal with it.

This change was made to be more flexible with asset tickers, but you're right that query params can be case-sensitive. I figured we can add configuration to the input params to specify whether the options are case-sensitive, but we don't have enough data to know which EAs need this right now (if any).

[vnavascues]

The only thing I'm not sure is the new case-insensitive options. Query parameters are case sensitive and with this change consumers don't have to be 100% accurate (as per README.md instructions) which means EA developers have to deal with it.

This change was made to be more flexible with asset tickers, but you're right that query params can be case-sensitive. I figured we can add configuration to the input params to specify whether the options are case-sensitive, but we don't have enough data to know which EAs need this right now (if any).

I think it is a great idea!

For instance, I've already dealt with multiple APIs that only support either checksum addresses or lowercase ones. Just imagine an EA having NFT projects whitelisted by address and using that list as options. It is a dummy example (better sending a uint instead of a stringified/bytes address, and then having that number mapped to the API expected address) but I think that it is enough to justify the case-sensitive config param.