Re: Transactionid in the ErrorResponse

[Carlos Santana]

We just need to find the floating spec and updated with the new schema.

Also `code` is not documented as top level API or documentation, didn't
find in the swagger doc [1]

+ 1 to change and adjust downstreams (i.e. CLI, etc..)

[1]
https://github.com/apache/incubator-openwhisk/blob/master/core/controller/src/main/resources/apiv1swagger.json


On Thu, Apr 19, 2018 at 12:04 PM Nick Mitchell  wrote:

> it's unlikely, for sure, but e.g. there may be a typescript spec floating
> around out there
>
> On Thu, Apr 19, 2018 at 11:58 AM, Rodric Rabbah  wrote:
>
> > It is changing the schema for the error response, but I'm having a hard
> > time thinking of a case where a client depends on the value of the "code"
> > (so that a change from int to string becomes a problem) - do you have an
> > example?
> >
> > Renaming "code" would be more worrisome to me, but apart from the CLI, I
> > suspect - with no evidence to back this up other than my intuition - that
> > it's not used that heavily.
> >
> > -r
> >
> > On Thu, Apr 19, 2018 at 11:51 AM, Nick Mitchell 
> > wrote:
> >
> > > this seems like a breaking API change. e.g. in nodejs `===` checks
> would
> > > break.
> > >
> > > On Thu, Apr 19, 2018 at 11:37 AM, Rodric Rabbah 
> > wrote:
> > >
> > > > Should we also rename “code”?
> > > >
> > > > I don’t see the value in using code: 0 and changing the schema should
> > be
> > > > fine and better in the long run.
> > > >
> > > > -r
> > > >
> > > > > On Apr 19, 2018, at 11:31 AM, Christian Bickel 
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > I'm currently working on a PR which basically moves the
> transactionId
> > > > generation from the controller to the entrypoint of the system. This
> is
> > > the
> > > > nginx or a frontdoor above.
> > > > > One change in this PR is to change the format of the tid from a
> > number
> > > > to a String.
> > > > > This works pretty well except one change, that could be seen by
> > users.
> > > > > If there is an error in our system, we return an error response
> with
> > a
> > > > short description and the tid. Until now the tid was a number, so the
> > > value
> > > > in the JSON has no quotes. With this change, the response message
> would
> > > > change, because the tid is a String.
> > > > > This means the response would change from
> > > > > ```
> > > > > {
> > > > > "error": "This is the description",
> > > > > "code": 123
> > > > > }
> > > > > ```
> > > > > to
> > > > > ```
> > > > > {
> > > > > "error": "This is the description",
> > > > > "code": "123"
> > > > > }
> > > > > ```.
> > > > >
> > > > > Do you agree, that this change would be OK?
> > > > > An alternative would be to always return a 0 and add an additional
> > > field
> > > > with our new tid-format.
> > > > >
> > > > > If there are no concerns, I'll go ahead and change the field from
> the
> > > > number to a String.
> > > > >
> > > > > Greetings
> > > > > Christian Bickel
> > > >
> > >
> >
>

Re: Transactionid in the ErrorResponse

[Nick Mitchell]

it's unlikely, for sure, but e.g. there may be a typescript spec floating
around out there

On Thu, Apr 19, 2018 at 11:58 AM, Rodric Rabbah  wrote:

> It is changing the schema for the error response, but I'm having a hard
> time thinking of a case where a client depends on the value of the "code"
> (so that a change from int to string becomes a problem) - do you have an
> example?
>
> Renaming "code" would be more worrisome to me, but apart from the CLI, I
> suspect - with no evidence to back this up other than my intuition - that
> it's not used that heavily.
>
> -r
>
> On Thu, Apr 19, 2018 at 11:51 AM, Nick Mitchell 
> wrote:
>
> > this seems like a breaking API change. e.g. in nodejs `===` checks would
> > break.
> >
> > On Thu, Apr 19, 2018 at 11:37 AM, Rodric Rabbah 
> wrote:
> >
> > > Should we also rename “code”?
> > >
> > > I don’t see the value in using code: 0 and changing the schema should
> be
> > > fine and better in the long run.
> > >
> > > -r
> > >
> > > > On Apr 19, 2018, at 11:31 AM, Christian Bickel 
> > > wrote:
> > > >
> > > > Hi,
> > > >
> > > > I'm currently working on a PR which basically moves the transactionId
> > > generation from the controller to the entrypoint of the system. This is
> > the
> > > nginx or a frontdoor above.
> > > > One change in this PR is to change the format of the tid from a
> number
> > > to a String.
> > > > This works pretty well except one change, that could be seen by
> users.
> > > > If there is an error in our system, we return an error response with
> a
> > > short description and the tid. Until now the tid was a number, so the
> > value
> > > in the JSON has no quotes. With this change, the response message would
> > > change, because the tid is a String.
> > > > This means the response would change from
> > > > ```
> > > > {
> > > > "error": "This is the description",
> > > > "code": 123
> > > > }
> > > > ```
> > > > to
> > > > ```
> > > > {
> > > > "error": "This is the description",
> > > > "code": "123"
> > > > }
> > > > ```.
> > > >
> > > > Do you agree, that this change would be OK?
> > > > An alternative would be to always return a 0 and add an additional
> > field
> > > with our new tid-format.
> > > >
> > > > If there are no concerns, I'll go ahead and change the field from the
> > > number to a String.
> > > >
> > > > Greetings
> > > > Christian Bickel
> > >
> >
>

Re: Transactionid in the ErrorResponse

[Tyson Norris]

I would prefer changing the schema from number to string. (And breaking any 
apps that use it) 

Until we have a better notion of release and versioning (including all API, db, 
and message schemas), I’m not too concerned about breaking API changes at this 
level (code is only useful for OW operators - so I have some doubts of whether 
it is actually used anywhere?). 

I don’t this this particular breaking change warrants a API version number 
change at this point in time, but I agree at some point in the future it will.  

Tyson
 

> On Apr 19, 2018, at 8:51 AM, Nick Mitchell  wrote:
> 
> this seems like a breaking API change. e.g. in nodejs `===` checks would
> break.
> 
> On Thu, Apr 19, 2018 at 11:37 AM, Rodric Rabbah  wrote:
> 
>> Should we also rename “code”?
>> 
>> I don’t see the value in using code: 0 and changing the schema should be
>> fine and better in the long run.
>> 
>> -r
>> 
>>> On Apr 19, 2018, at 11:31 AM, Christian Bickel 
>> wrote:
>>> 
>>> Hi,
>>> 
>>> I'm currently working on a PR which basically moves the transactionId
>> generation from the controller to the entrypoint of the system. This is the
>> nginx or a frontdoor above.
>>> One change in this PR is to change the format of the tid from a number
>> to a String.
>>> This works pretty well except one change, that could be seen by users.
>>> If there is an error in our system, we return an error response with a
>> short description and the tid. Until now the tid was a number, so the value
>> in the JSON has no quotes. With this change, the response message would
>> change, because the tid is a String.
>>> This means the response would change from
>>> ```
>>> {
>>> "error": "This is the description",
>>> "code": 123
>>> }
>>> ```
>>> to
>>> ```
>>> {
>>> "error": "This is the description",
>>> "code": "123"
>>> }
>>> ```.
>>> 
>>> Do you agree, that this change would be OK?
>>> An alternative would be to always return a 0 and add an additional field
>> with our new tid-format.
>>> 
>>> If there are no concerns, I'll go ahead and change the field from the
>> number to a String.
>>> 
>>> Greetings
>>> Christian Bickel
>> 

Re: Transactionid in the ErrorResponse

[Rodric Rabbah]

It is changing the schema for the error response, but I'm having a hard
time thinking of a case where a client depends on the value of the "code"
(so that a change from int to string becomes a problem) - do you have an
example?

Renaming "code" would be more worrisome to me, but apart from the CLI, I
suspect - with no evidence to back this up other than my intuition - that
it's not used that heavily.

-r

On Thu, Apr 19, 2018 at 11:51 AM, Nick Mitchell  wrote:

> this seems like a breaking API change. e.g. in nodejs `===` checks would
> break.
>
> On Thu, Apr 19, 2018 at 11:37 AM, Rodric Rabbah  wrote:
>
> > Should we also rename “code”?
> >
> > I don’t see the value in using code: 0 and changing the schema should be
> > fine and better in the long run.
> >
> > -r
> >
> > > On Apr 19, 2018, at 11:31 AM, Christian Bickel 
> > wrote:
> > >
> > > Hi,
> > >
> > > I'm currently working on a PR which basically moves the transactionId
> > generation from the controller to the entrypoint of the system. This is
> the
> > nginx or a frontdoor above.
> > > One change in this PR is to change the format of the tid from a number
> > to a String.
> > > This works pretty well except one change, that could be seen by users.
> > > If there is an error in our system, we return an error response with a
> > short description and the tid. Until now the tid was a number, so the
> value
> > in the JSON has no quotes. With this change, the response message would
> > change, because the tid is a String.
> > > This means the response would change from
> > > ```
> > > {
> > > "error": "This is the description",
> > > "code": 123
> > > }
> > > ```
> > > to
> > > ```
> > > {
> > > "error": "This is the description",
> > > "code": "123"
> > > }
> > > ```.
> > >
> > > Do you agree, that this change would be OK?
> > > An alternative would be to always return a 0 and add an additional
> field
> > with our new tid-format.
> > >
> > > If there are no concerns, I'll go ahead and change the field from the
> > number to a String.
> > >
> > > Greetings
> > > Christian Bickel
> >
>

Re: Transactionid in the ErrorResponse

[Rodric Rabbah]

Should we also rename “code”?

I don’t see the value in using code: 0 and changing the schema should be fine 
and better in the long run. 

-r

> On Apr 19, 2018, at 11:31 AM, Christian Bickel  wrote:
> 
> Hi,
> 
> I'm currently working on a PR which basically moves the transactionId 
> generation from the controller to the entrypoint of the system. This is the 
> nginx or a frontdoor above.
> One change in this PR is to change the format of the tid from a number to a 
> String.
> This works pretty well except one change, that could be seen by users.
> If there is an error in our system, we return an error response with a short 
> description and the tid. Until now the tid was a number, so the value in the 
> JSON has no quotes. With this change, the response message would change, 
> because the tid is a String.
> This means the response would change from
> ```
> {
> "error": "This is the description",
> "code": 123
> }
> ```
> to 
> ```
> {
> "error": "This is the description",
> "code": "123"
> }
> ```.
> 
> Do you agree, that this change would be OK?
> An alternative would be to always return a 0 and add an additional field with 
> our new tid-format.
> 
> If there are no concerns, I'll go ahead and change the field from the number 
> to a String.
> 
> Greetings
> Christian Bickel