Remove uninitialized ActiveVersion field from Service struct?

[asobrien]

The fastly client's GetService() method returns a Service struct which includes an ActiveVersion field, however the Fastly API docs for the GET /service/{service_id} endpoint (which the method in question utilizes) does not include an active_version field in its response:

HTTP/1.1 200 OK
Content-Type: application/json

{
  "comment": "",
  "created_at": "2020-04-27T19:40:49.000Z",
  "customer_id": "x4xCwxxJxGCx123Rx5xTx",
  "deleted_at": null,
  "id": "SU1Z0isxPaozGVKXdv0eY",
  "name": "test-service",
  "publish_key": "3c18bd0f5ada8f0cf54724d86c514a8eac4c9b75",
  "type": "vcl",
  "updated_at": "2020-04-27T19:40:49.000Z",
  "versions": [
    {
      "created_at": "2020-04-09T18:14:30.000Z",
      "updated_at": "2020-04-09T18:15:30.000Z",
      "deleted_at": null,
      "active": true,
      "comment": "",
      "deployed": true,
      "locked": false,
      "number": 1,
      "staging": false,
      "testing": false,
      "service_id": "SU1Z0isxPaozGVKXdv0eY"
    }
  ]
}

The consequence of this is that the Service struct advertises an ActiveVersion in the docs (e.g., godoc) although it will never be initialized and will always be equal to the underlying type's zero-value (i.e., 0). The fact that this unused public field is part of the Service struct can be misleading.

(I stumbled upon this issue by using the ActiveVersion in a Service struct which led to downstream errors. The GetServiceDetails() method and its returned ServiceDetail struct worked as expected.)

This all leads to following question:

Should the Service struct's ActiveVersion field be removed?

Removing it does introduce a breaking change. However, any end users rely on the ActiveVersion field from a Service would likely be dealing with unexpected behavior in any case. Note that none of the API's service endpoints return a active_version key in their response. I'd advocate for the field's removal (I did open this issue after all) but I'm curious what the maintainers/community have to say given the breaking change that would be introduced.

[Integralist]

👋🏻 @asobrien

Thanks for opening this issue. I'm reaching out to our API team to find out if active_version should indeed be returned by the API or whether this is a mistake on the part of the Go-Fastly client.

I'll update you once I have some feedback.

[Integralist]

Hi @asobrien thanks for your patience.

After some investigation (which also revealed that parts of the API documentation was incorrect) it would seem that the underlying confusion stems from the reuse of the Service struct across multiple endpoints.

For example, ListService unmarshals its data into the Service struct much like GetService does. The API response for listing a service does include a version key that identifies which listed service version is the activated one (hence this key is then mapped to the ActiveVersion field in the Service struct).

You are indeed correct that the API response data to GetService does not have a version key, and so when it unmarshals the data into the Service struct the ActiveVersion field will show up with the type's zero value.

I don't necessarily think this is a big concern, as the zero value in this case would indicate there's no active version information available, and for GetService that would indeed be correct (i.e. there is no active version info provided by the API).

It would be nice if we didn't have to include the ActiveField field but that would require having to have multiple distinct Service<N> types for each API endpoint, which I feel would be overkill in this situation.

With that in mind I'm inclined not to make any changes at this point in time.

[asobrien]

Thanks for the response @Integralist.

For example, ListService unmarshals its data into the Service struct much like GetService does. The API response for listing a service does include a version key that identifies which listed service version is the activated one (hence this key is then mapped to the ActiveVersion field in the Service struct).

I just verified that this is indeed true. I initially thought ActiveVersion wasn't initialized here as the example response in the API documentation for the GET /service endpoint does not include version as one of the keys in the response. (Note: only the GET /service/{service_id}/domain response show the use of the version key.)

Ultimately, I think the source of confusion stems from API documentation for the Service resource. I'd agree that it doesn't make sense that to return multiple distinct Service<N> types for each endpoint, so the burden of validity of an endpoint's response falls on the API docs--without that an end user is left to validate the data model for each endpoint on their own.

... I'm inclined not to make any changes at this point in time.

I'd agree with this given that ActiveVersion is not entirely superfluous in the Service struct. I'm going to close this issue since there's nothing actionable with respect to this codebase. If you can bring up my concern about how responses are reflected in the API docs with the appropriate team, I'd greatly appreciate it.