@mik-laj the in openapi spec, the PATH method for variable takes the following body:

PATCH /api/v1/variables/foo
{"key": "foo", "value": "bar"}

I think the key in request body is redundant, should we update the api spec to remove it?

I think the key in request body is redundant, should we update the api spec to remove it?

The structure of this request body results from the fact that it is based on the Variable schema.

    VariableCollectionItem:
      # Divided into two schemas for sensitive data protection
      type: object
      properties:
        key:
          type: string

    VariableCollection:
      type: object
      properties:
        variables:
          type: array
          items:
            $ref: '#/components/schemas/VariableCollectionItem'

    Variable:
      allOf:
        - $ref: '#/components/schemas/VariableCollectionItem'
        - type: object
          properties:
            value:
              type: string

The same schema is used to create as well as edit elements, so both fields must exist. We can create a new scheem that will not contain this one field, but it will complicate the API.

This field should be immutable, which is well described in the AIP.

When a service receives an immutable field in an update request (or similar), even if included in the update mask, the service should ignore the field if the value matches, but should error with INVALID_ARGUMENT if a change is requested.

https://google.aip.dev/203

Unfortunately, OpenAPI has no support for immutable fields, so we have some confusion in the specification definition. I think we can add an additional description to explain this.

What do you think about it?

When a service receives an immutable field in an update request (or similar), even if included in the update mask, the service should ignore the field if the value matches, but should error with INVALID_ARGUMENT if a change is requested.

Cool, my implementation already matches this behavior.

Unfortunately, OpenAPI has no support for immutable fields, so we have some confusion in the specification definition. I think we can add an additional description to explain this.

Yeah, I think it would be helpful to document this in the spec. How would you want this to be documented?

Nitpicking here, but explicitly testing the out the schema's serialization and de-serialization could be helpful once we start refactoring the code. @houqp, what do you think ?

@OmairK the api tests should have already covered serialization and deserialization code path, no? Do you have an example in mind where a refactor will cause issue that won't be caught by the api tests but can be caught by testing schema module itself?

@OmairK the api tests should have already covered serialization and deserialization code path, no? Do you have an example in mind where a refactor will cause issue that won't be caught by the api tests but can be caught by testing schema module itself?

I think, writing tests improve code coverage?

@OmairK the api tests should have already covered serialization and deserialization code path, no? Do you have an example in mind where a refactor will cause issue that won't be caught by the api tests but can be caught by testing schema module itself?

Yes, I think you are right :) Initially we were dealing with only read endpoints so we had to explicitly test the de-serialization, which you are already covering in create/update endpoints.

I think, writing tests improve code coverage?

@ephraimbuddy No not necessarily, my understanding of code coverage is based on this. IMO it wouldn't matter much in this case, both serialization and de-serialization is already covered in the CRUD end points.

I think, writing tests improve code coverage?

As @OmairK already mentioned, duplicated tests that test the same code path won't increase code coverage. In fact it will make future refactoring harder because you will have to change more than one tests whenever you introduce a behavior change.

If a code path can be covered by end to end api test, that should be preferred. It's better to test on expected behavior of the system instead of implementation details.

Yeah, I think it would be helpful to document this in the spec. How would you want this to be documented?

For now I think it's enough to add a note in the specification. "This field is immutable". I will add a general description of what the invariant field means at the beginning of the documentation later.