More public documentation/guidance on each contract test

[nina-ctrlv]

We provide a contract for handlers to follow in the docs, but it will be useful for providers to know what each individual test within the contract tests is trying to accomplish, what the inputs are for the test, and what the expected outcome is without having to read through the code of the test. Might also be useful to have more context on the test failures when we're reporting the results as well (ie why it's bad the test is failing, suggestions on how to fix it).

[advaj]

+1 - As someone who is migrating resources at scale. From personal experience, we have come up with some notes like - But if the tests have suggestions and what has failed specifically, that would be very useful.

1. Delete Handlers should not return a Model. So, when the Handler is returning a Success Progress Event, make sure that the Resource Model attribute is set to null. 
2. The Update, Read and Delete Handlers should contain a check for the existence of a Primary Identifier in the Model. If the Primary Identifier doesn’t exist, then throw a NotFound Error Code. 
3. If your Schema contains create only properties, make sure you include a check in the Update Handler for verifying if these properties are not being changed. 

Similarly, most contract tests check for something specific like this. Hence, some documentation about what each test is checking for and a potential resolution would be very useful. It would save us a ton of time as well. In some cases, it is taking us more time to fix contract tests than write the handler code.

[PatMyron]

Clearer contract test names themselves are needed. I fixed a ListHandler to pass a contract test without list in the name; seems unintuitive even if more documentation existed

[anshikg]

Re-opening this issue. It looks like we missed the documentation release here

[PatMyron]

3. If your Schema contains create only properties, make sure you include a check in the Update Handler for verifying if these properties are not being changed.

Why should every UpdateHandler handle this for every createOnlyProperty? The point of createOnlyProperties was so we didn't have to re-implement this

2. The Update, Read and Delete Handlers should contain a check for the existence of a Primary Identifier in the Model. If the Primary Identifier doesn’t exist, then throw a NotFound Error Code.

Again, if this always must be done, why rely on all these Handlers implementing this correctly anyway?

1. Delete Handlers should not return a Model. So, when the Handler is returning a Success Progress Event, make sure that the Resource Model attribute is set to null.

Again, why does every DeleteHandler need to do the same thing to the ResourceModel when returning a successful ProgressEvent?

---

All seem better handled levels above individual resource providers

[nina-ctrlv]

Why should every UpdateHandler have to handle this for every createOnlyProperty? I thought the point of createOnlyProperties was so we didn't have to worry about this

Completely agree. These contract tests should be removed this logic should be handled by cloudformation.

Wondering the same for this as well. If this always must be done, why rely on all of these Handlers implementing this correctly anyway?

In terms of checking whether the resource exists, I do think it should be handled by each handler. The error codes and exceptions thrown will be unique for whatever service it is. Providers will still need to make the translation so cfn understands. Cloudformation could use the read handler in order check before each action but the provider still needs to implement it correctly with the read handler.

Same for this as well. Why does every DeleteHandler need to do the exact same thing to the ResourceModel when returning a successful ProgressEvent?

I agree as well 👍

[PatMyron]

In terms of checking whether the resource exists, I do think it should be handled by each handler. The error codes and exceptions thrown will be unique for whatever service it is. Providers will still need to make the translation so cfn understands. Cloudformation could use the read handler in order check before each action but the provider still needs to implement it correctly with the read handler.

In general, I agree with this if a potentially valid primaryIdentifier is in the ResourceModel but the resource just does not exist. But if the identifier isn't even in the ResourceModel or doesn't even match the identifier constraints, I don't think we need individual Update, Read and Delete handlers to handle that. Update, Read and Delete Handlers should not be called without a potentially valid identifier and downstream service calls shouldn't be necessary to know a resource should not be found without a potentially valid identifier

[advaj]

Also, if Physical ID = Primary Identifier, then Updates and Deletes will always have an ID present when they are invoked? Why should this be checked at all?

[benbridts]

Update, Read and Delete Handlers should never be called without a potentially valid primaryIdentifier and downstream service calls shouldn't be necessary to know a resource should not be found without a potentially valid primaryIdentifier

I agree. However, it does make sense to phrase it as "If the Update, Read and Delete Handlers are invoked for a not-existing primaryIdentifier, they should throw a NotFound Error Code."

As opposed to:

• throwing another error
• trying to identify the resource based on other properties

[rjlohan]

I'll also just throw in the note that we also optionally include additionalIdentifiers in schemas. If these are passed instead of a primaryIdentifier on an Update, Read or Delete they should be treated as if they are the primaryIdentifier, however if a primaryIdentifier is also present, it should take precendence (though in practice these values should always refer to the same resource instance).

The intent here was to support cases where more than one identifier may exist - for example an AWS::S3::Bucket can uniquely be identified by any of its Name, Url and Arn properties.

[benbridts]

I'll also just throw in the note that we also optionally include additionalIdentifiers in schemas. If these are passed instead of a primaryIdentifier on an Update, Read or Delete they should be treated as if they are the primaryIdentifier, however if a primaryIdentifier is also present, it should take precendence (though in practice these values should always refer to the same resource instance).

So in theory there is a conflict with the contract, because these cannot be both true:

• If the Primary Identifier doesn’t exist, then throw a NotFound Error Code
• If the primaryIdentifier doesn’t exist, use the additionalIdentifiers instead

In practice it probably does not matter at this point (since the primaryIdentifier is always set). However the possibility to do a read of a resource by passing one of the additionalIdentifiers could have some very nice use cases.

In any case, there is definitely some improvement of that contract test possible :)

[kddejong]

+1 to the tests having better information and details around them. Trying to trouble shoot failing tests requires a lot of code diving to figure out what they are looking for. The abbreviated errors you get are not very helpful at all and the debug/verbose logs are lacking the needed details.

+1 additionally I keep running into a bunch of contracts that I think should be managed by the service. I'm not sure its good for my code to handle things that the schema should have handled (example: min/max on an integer). I can't tell if this is a code bug or as designed yet...

[anshikg]

3. If your Schema contains create only properties, make sure you include a check in the Update Handler for verifying if these properties are not being changed.

Why should every UpdateHandler have to handle this for every createOnlyProperty? I thought the point of createOnlyProperties was so we didn't have to worry about this

I don't think this is expected from the handlers any more. Right now, the Contract test hit the Prod endpoint and this condition should be handled by the langugae specific plugin. If this is not handled by the languga eplugin please cut us an issue in the respective language plugin repo.e.g: Python Java

2. The Update, Read and Delete Handlers should contain a check for the existence of a Primary Identifier in the Model. If the Primary Identifier doesn’t exist, then throw a NotFound Error Code.

Wondering the same for this as well. If this always must be done, why rely on all of these Handlers implementing this correctly anyway?

This check should not be performed on the handlers. Please cut us a ticket if this is the case for you resource

1. Delete Handlers should not return a Model. So, when the Handler is returning a Success Progress Event, make sure that the Resource Model attribute is set to null.

Same for this as well. Why does every DeleteHandler need to do the exact same thing to the ResourceModel when returning a successful ProgressEvent?

This the current ask on the handlers, because if the handler is not successful in deleting the resource we expect the resource to be returned. Please let me know if you have concerns about implementing this on the handlers.

[benbridts]

This the current ask on the handlers, because if the handler is not successful in deleting the resource we expect the resource to be returned. Please let me know if you have concerns about implementing this on the handlers.

• There is currently nothing in the contract about requiring a model when the operation fails (it might be in the tests?)
• Even if it was. Requiring a model on failure, should not mean that a model should be absent on succes. The caller can look at the returned OperationStatus and ignore the model if it decides to do so
• If implemented that way, it might be a good idea to make the fact that it's optional explicit: eg "When the delete handler returns SUCCESS, the model SHOULD be ignored, even if it's returned in the ProgressEvent."