x-nullable does not work for response headers

[graywolf-at-work]

Problem statement

x-nullable is not respected for response headers.

Swagger specification

info:
  title: Example
  version: 1.0.0
schemes:
- http
swagger: "2.0"

paths:
  /foo:
    get:
      responses:
        200:
          description: All ok.
          headers:
            bar:
              type: string
              x-nullable: true

Steps to reproduce

+   $ swagger generate server -q
+   $ cat restapi/operations/get_foo_responses.go | grep Bar | grep json:
        Bar string `json:"bar"`

You can see that Bar is string. But it should be *string.

Partial analysis

The template for responses seems handle the case of nullable header just fine, but the IsNullable information does not get propagated to the template:

+   $ swagger generate server -q --dump-data | jq .Operations[0].SuccessResponses[0].Headers[0]
.IsNullable
false
+   $ swagger generate server -q --dump-data | jq .Operations[0].SuccessResponses[0].Headers[0]
{
  "IsAnonymous": false,
  "IsArray": false,
  "IsMap": false,
  "IsInterface": false,
  "IsPrimitive": true,
  "IsCustomFormatter": false,
  "IsAliased": false,
  "IsNullable": false,
  "IsStream": false,
  "IsEmptyOmitted": false,
  "IsJSONString": false,
  "IsBase64": false,
  "IsTuple": false,
  "HasAdditionalItems": false,
  "IsComplexObject": false,
  "IsBaseType": false,
  "HasDiscriminator": false,
  "GoType": "string",
  "Pkg": "",
  "PkgAlias": "",
  "AliasedType": "",
  "SwaggerType": "string",
  "SwaggerFormat": "",
  "Extensions": null,
  "ElemType": null,
  "IsMapNullOverride": false,
  "IsSuperAlias": false,
  "HasValidations": false,
  "Required": true,
  "MaxLength": null,
  "MinLength": null,
  "Pattern": "",
  "MultipleOf": null,
  "Minimum": null,
  "Maximum": null,
  "ExclusiveMinimum": false,
  "ExclusiveMaximum": false,
  "Enum": null,
  "ItemsEnum": null,
  "MinItems": null,
  "MaxItems": null,
  "UniqueItems": false,
  "HasSliceValidations": false,
  "NeedsSize": false,
  "Package": "operations",
  "ReceiverName": "o",
  "IndexVar": "i",
  "ID": "Bar",
  "Name": "bar",
  "Path": "\"bar\"",
  "ValueExpression": "o.Bar",
  "Title": "",
  "Description": "",
  "Default": null,
  "HasDefault": false,
  "CollectionFormat": "",
  "Child": null,
  "Parent": null,
  "Converter": "",
  "Formatter": "",
  "ZeroValue": "\"\""
}

I do not know enough about go-swagger to debug this further.

Environment

+   $ swagger version
version: v0.22.0
commit: 5773cbe63c3f459b23ed73ad8b482389ddf46cb4

+   $ go version
go version go1.14 linux/amd64

+   $ cat /etc/os-release
NAME="Arch Linux"
PRETTY_NAME="Arch Linux"
ID=arch
BUILD_ID=rolling
ANSI_COLOR="0;36"
HOME_URL="https://www.archlinux.org/"
DOCUMENTATION_URL="https://wiki.archlinux.org/"
SUPPORT_URL="https://bbs.archlinux.org/"
BUG_REPORT_URL="https://bugs.archlinux.org/"
LOGO=archlinux
+   $ uname -a
Linux wsshowmax 5.5.8-arch1-1 #1 SMP PREEMPT Fri, 06 Mar 2020 00:57:33 +0000 x86_64 GNU/Linux

[fredbi]

Yes this is a bug. This call in types.go should inspect the extensions before return the resolved type. https://github.com/go-swagger/go-swagger/blob/468a61360689c96f4e1a7350feeadf8eebb44b50/generator/types.go#L125

[fredbi]

@graywolf-at-work I'd need your input on this.

I've dug a bit into that one and found out that the x-nullable extension is not supported for simple types in general, that is parameters as well as headers.

So basically, this is not really a bug, but a missing feature. It is not hugely complicated to fix, but I am wondering how far this rabbit hole is going.

If we want to enable this, we should be at least consistent and enable this to parameters with simple schemas just as well.

I run a couple of tests with this and they showed that the response template is only half-baked to support pointers there. Parameters should be okay since there is a well-known use case with pointers.

I also realized that x-go-name is not supported for response headers (although it is for parameters)...

For simple params, pointer layout is adopted for all non-required parameters. Response headers (simple swagger types) cannot be specified as required, and we don't make a difference between a zero value here or no header.

Overall, I understand that this is a limitation in go-swagger (i.e. not being able to customize pointer layout for simple swagger types) but I am not sure about what value this brings (relative to the extra amount of work here, which is moderate, but not trivial).

Beyond just being consistent with go-swagger extensions across all spec construct, what could be a useful, valid use case to support pointers in this case?

@casualjim your grain of salt here could also help me figure this out.

[graywolf-at-work]

Use case is that in few places we need to be able to tell apart empty and not set header. However since I've managed to find (arguably really ugly) workaround

func outHeaderValue(val string) string {
        if val == "" {
                val = " "
        }
        return val
}

resp.Bar = outHeaderValue(barValue)

given that ^ works, it probably is not really high priority from my end, however if there are no immediate plans to fix (or rather implement I guess) this, it should at least be mentioned in documentation. Not sure where would be appropriate place though.

Response headers (simple swagger types) cannot be specified as required,

Is this intentional design decision? It would be nice to be able to declare output header as mandatory (since they are actually mandatory in our case).

[fredbi]

@graywolf-at-work ok I get that. Not so unusual after all.

Response headers cannot be required

This is as per swagger 2.0 schema. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#headerObject

My problem here is that if we want to be consistent with parameters, and since response headers are never required (but may have a default value), response headers should be most of the time rendered as a pointer. Not such a big deal, but a big impact for all users of the current codegen...

A workable trade-off could be to leave the current mapping unchanged and add support to x-nullable for parameters and response headers, as you have tried to do.

[fredbi]

Note that the generated server never sends a header with a zero value for the type.

Another point to consider is that, on the generated client, headers are retrieved as values, the absence of header is equivalent to having a zero value (e.g "" in your case). This is basically how the net/http lib works with headers.

[graywolf-at-work]

A workable trade-off could be to leave the current mapping unchanged and add support to x-nullable for parameters and response headers, as you have tried to do.

I think that would be perfectly work-able solution for us.

Another point to consider is that, on the generated client, headers are retrieved as values, the absence of header is equivalent to having a zero value (e.g "" in your case).

Actually we are not using the generated client so that is not an issue for us at the moment. Once we start it in deed is another issue that will need to be solved :/

[casualjim]

So you're saying that the required property for headers in your spec isn't working?

required: true

Because afaict this issue should be about required validations and not about nullable fields.

[fredbi]

@casualjim what I am saying here is that there is no required property for headers in responses. This is as per the swagger 2.0 schema. You can do that for headers in params, and this works well (pointer decision is just the other way around: "required" <=> no pointer).

Indeed @graywolf-at-work may have a point here, but the use case would be only for either generated clients working with a preexisting server or other clients working with a generated server.

In our "generated world", all works for response headers as if the zero value for a type didn't exist at all and such values are never transported over the wire. Ex:

• "" <=> no header
• int(0) <=> no header
• strfmt.Time{} <=> no header
• etc

Since the runtime is assuming this (and so is net/http.Header for strings), I am wondering if we really did miss something here. Certainly not for strings. Maybe regarding zero values for other types... Supporting x-nullable is not a big deal (I made a quick experiment with it) , I am just wary of not bringing some breaking change for a corner case.

[graywolf]

I would just like to add that even in net/http you can check if header is present or not like

        seen := false
        for k, v := range resp.Header {
                if strings.EqualFold(k, "some-header") {
                        seen = true
                }
        }

but yes, in the "generated world" there is no way to do such a check.

[casualjim]

@fredbi I don't think that is true. The spec says required is allowed on headers. What isn't there is AllowEmptyValue

@graywolf imo it's far better design to use a value and absence thereof. Because relying on behavior that deals with absence of a key can be tricky. Especially when you consider the varying implementations of http that are out there. In go there happens to be a prevalent one but in java there are many, each of them has made slightly different choices.

[fredbi]

@casualjim this is for headers in params, which work fine. I am talking about response headers.

[fredbi]

I believe the only sound thing we can do here is to support x-nullable for simple params. That would give some leeway to address specific situations, in particular with heterogeneous implementations for clients and servers. @casualjim does it sound reasonable to you? @graywolf-at-work if you are willing to help here, I can share the rapid test I've set up, so you have most code hotspots needed to solve the issue.

[casualjim]

it seems like just the isnullable property is never set. the template already supports it

So I guess the fix goes here: https://github.com/go-swagger/go-swagger/blob/master/generator/types.go#L70 or here https://github.com/go-swagger/go-swagger/blob/master/generator/types.go#L124

[graywolf-at-work]

@casualjim that was my impression as well but I was not sure

@fredbi sure I can give it a try