contract_tests: contract_create_read_success and primaryIdentifiers in responses

[SHxKM]

When a customer wants to create a our resource, he sends a request like so:

{
    "ourObj": {
        "someProp": "someValue"
    }
}

And our API, upon successful creation, will return the following:

{
    "ourObj": {
        "someProp": "someValue",
        "id": "ourJustGeneratedObjId"
    }
}

It seems that there is no way at all to pass the contract-tests with this schema. Passing a hard-coded value for ourObj.id does not make sense, as the delete test will fail.

I've worked around the contract_create_delete failure by copying ourObj.id to another property in the schema:

{
    "CfnId": "ourJustGeneratedObjId",
    "ourObj": {
        "someProp": "someValue",
        "id": "ourJustGeneratedObjId"
     }
}

CfnId is set as the primaryIdentifier.

This way, the model sent in the request is returned back in the response - none have the ourObj.id. However, our customers may be relying on the existence of this id inside ourObj.

What can be done about this?

[benbridts]

Can you share a snippet of your schema?

Is your issue that id is a nested property, or that you need it to be readonly?

[SHxKM]

Hey @benbridts, thanks for the quick reply!

id is both nested under ourObj and should be readOnly.

Here's an obfuscated and trimmed schema:

{
  "typeName": "SomeComp::SomeProduct::SomeObj",
  "description": "SomeComp SomeProduct Provider",
  "properties": {
    "cfnObjId": {
      "type": "string"
    },
    "ourObj": {
      "type": "object",
      "properties": {
        "objId": {
          "type": "string"
        },
        "name": {
          "description": "The name of the obj",
          "type": "string"
        }
      }
    }
  },
  "primaryIdentifier": [
    "/properties/cfnObjId"
  ],
  "readOnlyProperties": [
    "/properties/cfnObjId"
  ],
  "handlers": {
    "create": {
      "permissions": [
        ""
      ]
    },
    "update": {
      "permissions": [
        ""
      ]
    },
    "delete": {
      "permissions": [
        ""
      ]
    },
    "read": {
      "permissions": [
        ""
      ]
    }
  },
  "additionalProperties": false
}

This is after the cfnObjId workaround that I've employed. I'd rather not have it in there and just rely on ourObj.objId but it was impossible to pass most tests with that in there.

[benbridts]

I haven't tried nested properties as primary identifier yet, but can't you remove the nesting (either by not including the objId from ourObj, or by removing ourObj completely?

As an aside: I've seen nested properties usually done with a definitions block (eg. https://github.com/WeAreCloudar/cloudformation-samples/blob/main/resource_providers/cloudar_codebuild_build/cloudar-codebuild-build.json#L6).

[SHxKM]

Thanks Ben. Both options are very undesirable as this is a product that’s in very wide use already, we just need it to pass tests at the moment as we have a certain requirement. I’m worried about breaking changes.

[SHxKM]

@benbridts by the way I'm not sure how removing the nesting would help, I still need to return the ID to my end-users on read (or simply set it on the model so the delete test will succeed). They won't send the ID on creation, so it seems that contract_create_read_success will fail anyway.

[SHxKM]

I think my issue is very similar if not identical to this one. https://github.com/aws-cloudformation/cloudformation-cli/issues/646

[benbridts]

I was assuming you could get all the information if you knew the id (since you have it as primary identifier).

Let's assume you use a schema like this (I skipped some parts):

{
  "typeName": "SomeComp::SomeProduct::SomeObj",
  "properties": {
    "Id": {"type": "string"},
    "Name": {"type": "string"}
  },
  "primaryIdentifier": ["/properties/Id"],
  "readOnlyProperties": ["/properties/Id"],
}

Your users would be using this template:

Resources:
  Something:
    Type: SomeComp::SomeProduct::SomeObj
    Properties:
      Name: TheName

• In the create handler you call your API, get the Id from the response, and return it in the model
• In the read handler CloudFormation includes the Id in the request (it's the primary identifier), so you call your API, extract the Name from the response and return it in the Model.

Depending on your API you might need a different PrimaryIdentifier, that part is dependent on how the underlying API looks

[SHxKM]

The primaryIdentifier is generated at creation time and does not exist before it, so users can’t possibly pass it.

[benbridts]

@SHxKM That's why it's a readOnlyProperty. The user wouldn't be able to pass it.

[SHxKM]

@benbridts OK. Yes. But the tests are still failing because the test I mentioned expects the input (what the user passed) to be like the output (what I return from ReadHandler) and that obviously can never be. See the threads I attached.

[benbridts]

I might be wrong here, but AFAIK the input used for the read-handler in contract_create_read_success is not what the user passes.

It should work like this (using my example model and template):

• CreateHandler is called with {"Name": "TheName"} and returns {"Name": "TheName", "Id": "id-12345"}
• ReadHandler is called with either {"Name": "TheName", "Id": "id-12345"} or {"Id": "id-12345"} (IIRC according to the contract it should be the second, but the test might include more properties)
• ReadHandler should return {"Name": "TheName", "Id": "id-12345"}

Assuming authentication isn't a problem (can be tricky if only the Id is passed to the ReadHandler), this seems possible?

[SHxKM]

Well that’s not what’s happening in the tests. create_read_success is expecting EXACTLY the same output from ReadHandler (at the read stage) as the input that was passed to CreateHandler (at the creation stage).

[benbridts]

Is it? I didn't have time to test it again, but if I look at the source code

def contract_create_read_success(created_resource, resource_client):
    input_model, created_model, _request = created_resource
    read_response = test_read_success(resource_client, created_model)
    test_input_equals_output(
        resource_client, input_model, read_response["resourceModel"]
    )

At first glance, it looks like the original input of the create handler (input_model) has to exactly match the output of the read handler (read_response["resourceModel"]). However, if I take a look at the test_input_equals_output function, I see the read_only_paths being removed from both, so Only the Name should be compared:

def test_input_equals_output(resource_client, input_model, output_model):
    pruned_input_model = prune_properties_from_model(
        input_model.copy(),
        set(
            list(resource_client.read_only_paths)
            + list(resource_client.write_only_paths)
        ),
    )

    pruned_output_model = prune_properties_from_model(
        output_model.copy(), resource_client.read_only_paths
    )

    pruned_output_model = prune_properties_if_not_exist_in_path(
        pruned_output_model, pruned_input_model, resource_client.create_only_paths
    )
    # ...

To be clear: I didn't test this, so there might be another issue somewhere (the code I'm pointing to might not be released yet, or I might be misunderstanding the code). However, as far as I can see there is nothing that explicitly prevents my proposed model

[SHxKM]

@benbridts first off, I really appreciate your help and the fact that you are looking into this thoroughly.

If you look at the thread that I linked above, issue number #646, You’ll see the exact code snippet that you are referring to in the context of the same test that we are talking about. And there seems to be an acknowledgment there that this test fails with it’s current structure.

[benbridts]

Oh, interesting!

There have been some changes to that file (most notably #681), so that was why I was confused (and it probably makes sense for someone from the CloudFormation team to take a closer look at the things that are happening in the test)

The good news is that a wrong test is probably easier to solve than an incompatibility in the schema

[SHxKM]

@benbridts - yep, I actually got the suite to pass by using a different version of cli-cloudformation, but only when the primaryIdentifier isn't nested..

[benbridts]

Awesome. The CloudFormation purist in me highly recommends not nesting anyway. For two reasons:

• It doesn't seem obvious/common to have a primary identifier (the thing that will be !Ref'ed) in a nested property
• CloudFormation doesn't have to look exactly like the API (take as an example the resources that combine mulitple API calls into one), and having the Resource have properties looks (from the limited example) a bit better to me.

If you want to keep the nesting, because that is what your customers expect, you can still only unnest the ID, as that will not be visible or specified to your users anyway (you could duplicate it too, but I don't see the added value of that from the example)

[SHxKM]

Thanks @benbridts.

...you can still only unnest the ID, as that will not be visible or specified to your users anyway

Can you elaborate on this please? I'm not sure I understand. One of the things we're worried about is backwards-compatibility, are you saying it's a non-issue?

[benbridts]

Currently this is what you're asking the users to write in CloudFormation:

Resources:
  Something:
    Type: SomeComp::SomeProduct::SomeObj
    Properties:
      ourObj:
        someProp: someValue
Outputs:
  Id:
    # note: this very likely not supported in CloudFormation
    Value: !GetAtt Something.ourObj.Id,
   AlsoId:
     # Ref retuns the primary identifier - CfnId in the schema
     Value: !Ref Something

If you don't specify the id in ourObj, but instead remove it completely and use CfnId as the primary identifier, you only lose the !GetAtt in that template. And if you do that, you might as well call it Id, because that is what it is

Backwards compatibility is always something to keep in mind, but your resource provider is a translation layer between the external API and what the user puts in their template, so they don't need to be a 1-1 mapping.

In fact, a lot of resource providers do some mutation. Eg:

• in CloudFormation properties usually are in upper CamelCase (PascalCase).
• in the CloudWatch Logs API the logGroupName attribute is lower camelCase (API link)
• the CloudFormation::Logs::LogGroup resource provider will do that mutation (schema link)

So the interface that you need to keep backwards-compatible is what you put in the schema, not the relationship between the schema and the api

[SHxKM]

Thanks @benbridts.

I'm keeping this open as nested primaryIdentifiers seem to fail the tests, which shouldn't happen.