[Bug]: A model containing an @header property doesn't keep that property when the object is generated

[trangevi]

Describe the bug

When I specify a model in typespec which contains a property with the @Header decorator, the generated code produces an object for the model which doesn't contain the header property. This is true regardless of whether the model is spread in an operation or not. I've repro'ed with a basic solution, below.

Reproduction

I put the typespec I used in a playground here, but I'm not exactly sure what pieces need to be included for it to compile, and felt like the C# code which was produced is relevant as well, so I've included that here.

So the typespec, with a model containing a header and a property:

model HeaderBugReproModel {
  @header("x-foo")
  foo: string;

  bar: int32;
}

And an operation asking for the model to be spread:

@actionSeparator("/")
@route("header/test")
op bugTest(...HeaderBugReproModel): void;

Produces (I removed most of the comments to make it smaller)

An anonymous model without the header property:

namespace Azure.AI.Inference
{
    /// <summary> The BugTestRequest. </summary>
    internal partial class BugTestRequest
    {
        private IDictionary<string, BinaryData> _serializedAdditionalRawData;

        internal BugTestRequest(int bar)
        {
            Bar = bar;
        }

        internal BugTestRequest(int bar, IDictionary<string, BinaryData> serializedAdditionalRawData)
        {
            Bar = bar;
            _serializedAdditionalRawData = serializedAdditionalRawData;
        }

        internal BugTestRequest()
        {
        }

        /// <summary> Gets the bar. </summary>
        public int Bar { get; }
    }
}

And a request method with the parameters spread, including the header

public virtual async Task<Response> BugTestAsync(string foo, int bar, CancellationToken cancellationToken = default)

If I change the operation to not spread the model

op bugTest(body: HeaderBugReproModel): void;

It produces

The model from the typespec, but without the header property

namespace Azure.AI.Inference
{
    /// <summary> The HeaderBugReproModel. </summary>
    public partial class HeaderBugReproModel
    {
        private IDictionary<string, BinaryData> _serializedAdditionalRawData;

        public HeaderBugReproModel(int bar)
        {
            Bar = bar;
        }

        internal HeaderBugReproModel(int bar, IDictionary<string, BinaryData> serializedAdditionalRawData)
        {
            Bar = bar;
            _serializedAdditionalRawData = serializedAdditionalRawData;
        }

        internal HeaderBugReproModel()
        {
        }

        public int Bar { get; }
    }
}

The anonymous model, now taking in the defined model instead of just the property

namespace Azure.AI.Inference
{
    /// <summary> The BugTestRequest. </summary>
    internal partial class BugTestRequest
    {
        private IDictionary<string, BinaryData> _serializedAdditionalRawData;

        internal BugTestRequest(HeaderBugReproModel body)
        {
            Argument.AssertNotNull(body, nameof(body));

            Body = body;
        }

        internal BugTestRequest(HeaderBugReproModel body, IDictionary<string, BinaryData> serializedAdditionalRawData)
        {
            Body = body;
            _serializedAdditionalRawData = serializedAdditionalRawData;
        }

        internal BugTestRequest()
        {
        }

        public HeaderBugReproModel Body { get; }
    }
}

And a request method which includes both the header and the model object

public virtual async Task<Response> BugTestAsync(string foo, HeaderBugReproModel body, CancellationToken cancellationToken = default)

Checklist

• Follow our Code of Conduct
• Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[iscai-msft]

1. With spread, tcgc is giving an anonymous model for the body with 1 prop: bar. Header foo is on the command line. This is correct behavior
2. If we group the model, tcgc returns the model with the header property on it as well (see existing test here). In this case, for c#, it appears the header is now missing because it's not on the method signature, and it's not in the generated model. Python still moves everything onto the method signature and this is by design. So @m-nash it appears that c# is dropping the header at some point

[JoshLove-msft]

Decide whether we want to generate the property and skip serialization, or if we want to collapse this into the method signature.

[m-nash]

If the model that contains the header is an "OptionsBag" then we leave it in the model and skip serialization. If the model that contains the header is NOT an "OptionsBag" then we move it to the signature.

In tsp an options bag would be indicated by a operation that takes the entire model instead of the spread of the model.

op foo(payload: Model): void

vs

op foo(...Model): void

vs output

op foo(): Model

The header property is included on the Model as long as it has at least read visibility.

[JoshLove-msft]

This also needs to be considered for response headers and whether the behavior should differ between request/response headers. We should get architect feedback.

This should also be a Spector scenario.

[JoshLove-msft]

2. In this case, for c#, it appears the header is now missing because it's not on the method signature, and it's not in the generated model.

This part isn't true. The header does appear on the signature in C#, but we want to decide whether we should change the behavior to more closely match the tsp.

[JoshLove-msft]

Given the fact that the model properties will no longer contain the request/response bodies as per the protocol definition, we should consider the concept of protocol vs convenience models as per @KrzysztofCwalina.

A protocol model represents the request/response body exactly, whereas a convenience model may contain additional things like headers. We would always generate protocol models, and sometimes there would be a separate convenience model. When implementing the support for generating header properties within a model, we should consider still generating an internal protocol model that does not have the extra property and use that when serializing/deserializing the body. The extra information from the header would be used to construct the convenience model, e.g. header + protocol model == convenience model. If we ever decide to make protocol models a public concept for use with protocol methods, we will be better positioned to do that if we generate internal protocol models now. The downside to having the separate models is we would have to do a copy when going from protocol to convenience and vice versa.

One other thing to note is that it is by design that header properties will not work inside of a model that is annotated with body, or for which there is no annotation (as body is the default location). If tracing is enabled, you will see a warning like this:

../C:/Users/jolov/repos/typespec/packages/http-client-csharp/generator/TestProjects/Local/Unbranded-TypeSpec/Unbranded-TypeSpec.tsp:103:3 - warning @typespec/http/metadata-ignored: header property will be ignored as it is inside of a @body property. Use @bodyRoot instead if wanting to mix.
> 103 |   requiredUnion: string | string[] | int32;

To enable header properties, the container needs to be decorated with @bodyRoot rather than @body.

[JoshLove-msft]

This is blocked on Azure/typespec-azure#2122

[JoshLove-msft]

Visibililty has been added for non-body properties in Azure/typespec-azure#2163

[ArcturusZhang]

So we are really filtering out those non-payload properties here:

typespec/packages/http-client-csharp/emitter/src/lib/type-converter.ts

Line 162 in 3d33cda

if (property.kind !== "property") {

I think "fixing" this requires quite a few changes in our generator.

1. Now our InputModelProperty only represents kind: property things in TCGC, therefore we need to update this to be a union to include other kinds of parameters such as kind: header, kind: path, kind: query.
2. Our request creation logic needs to update to collect those non-payload parameters inside the model
3. Our serialization logic needs to update to properly deal with those non-payload parameters, and it should only write those payload properties
4. Once we change this, we might get a lot of changes from our azure libraries - how we fix those? I think the best solution could be to change their specs so that those non-payload properties are defined outside the model - but this requires huge changes because majority of these cases were introduced by templates.