search

aws-greengrass / aws-greengrass-shadow-manager

A GreengrassV2 Component that provides offline device shadow documents and optional synchronization to the IoT device shadow service.
Apache License 2.0
9 stars 6 forks source link

fix: update necessary if shadow cleared #196

Closed jcosentino11 closed 1 year ago

jcosentino11 commented 1 year ago

Issue #, if available:

Description of changes: Ensure the cloud update document {"state": null} isn't ignored.

Currently, if a null cloud shadow update accepted document is received, {"state": null}, shadow manager will discard the update, because it thinks no change was made to the cloud state after merging the existing cloud document with {"state": null}:

boolean isUpdateNecessary(JsonNode baseDocument, JsonNode update) {
    JsonNode merged = baseDocument.deepCopy();
    JsonMerger.merge(merged.get(SHADOW_DOCUMENT_STATE), update.get(SHADOW_DOCUMENT_STATE));
    return !baseDocument.equals(merged);
}

To disambiguate between null and empty, we can explicitly check the update request, {"state": null}, within isUpdateNecessary.

In order to enable this comparison, we need to allow ShadowDocument#state to be null, which required some refactoring to support.

Why is this change necessary: To allow null cloud shadow updates {"state": null} to be accepted by shadow manager, which will subsequently clear the local shadow.

How was this change tested: A new integration test has been added, and we have UAT coverage as well, which is how the issue was initially discovered.

Any additional information or context required to review the change:

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

MikeDombo commented 1 year ago

Test?

github-actions[bot] commented 1 year ago

Unit Tests Coverage Report

File Coverage Lines Branches
All files 83% 89% 78% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.MergedFullShadowSyncRequest 21% 26% 15% :x:
com.aws.greengrass.shadowmanager.sync.model.LocalUpdateSyncRequest 79% 85% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.Direction 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalDeleteSyncRequest 99% 98% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.FullShadowSyncRequest 84% 97% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudDeleteSyncRequest 80% 78% 83% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.DirectionWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.OverwriteCloudShadowRequest 98% 95% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.BaseSyncRequest 81% 93% 69% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.OverwriteLocalShadowRequest 98% 95% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudUpdateSyncRequest 74% 76% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDatabase 2% 2% 0% :x:
com.aws.greengrass.shadowmanager.ShadowManager 74% 78% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDAOImpl 96% 97% 96% :white_check_mark:
com.aws.greengrass.shadowmanager.AuthorizationHandlerWrapper 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.PubSubIntegrator 89% 93% 86% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManager$1 43% 43% 0% :x:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractDeleteThingShadowOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractListNamedShadowsForThingOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractUpdateThingShadowOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractGetThingShadowOperationHandler 67% 67% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonMerger 94% 98% 89% :white_check_mark:
com.aws.greengrass.shadowmanager.util.SyncNodeMerger 91% 98% 84% :white_check_mark:
com.aws.greengrass.shadowmanager.util.DataOwner 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.ShadowWriteSynchronizeHelper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.Validator 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonUtil 94% 99% 90% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.BaseRequestHandler 77% 77% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowRequestHandler 88% 100% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.PubSubClientWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.IpcRateLimiter 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter 94% 89% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowRequestHandler 99% 99% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter$1 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowRequestHandler 91% 95% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.ListNamedShadowsForThingIPCHandler 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.Operation 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.PubSubRequest 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.PeriodicSyncStrategy 63% 77% 50% :x:
com.aws.greengrass.shadowmanager.sync.strategy.SyncStrategyFactory 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.BaseSyncStrategy 93% 97% 89% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.RealTimeSyncStrategy 93% 86% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientFactory 55% 85% 25% :x:
com.aws.greengrass.shadowmanager.sync.RequestBlockingQueue 96% 99% 93% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.SyncHandler 71% 88% 53% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientWrapper 91% 91% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestMerger 79% 77% 81% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.CloudDataClient 74% 72% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ShadowSyncConfiguration 87% 93% 82% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ThingShadowSyncConfiguration 76% 78% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.ShadowDocSizeConfiguration 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.RateLimitsConfiguration 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.ComponentConfiguration 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.StrategyType 86% 92% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.Strategy 90% 100% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ResponseMessageBuilder 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowDocument 85% 90% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowStateMetadata 93% 98% 89% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ErrorMessage 75% 100% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowRequest 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowState 78% 81% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.model.LogEvents 100% 100% 0% :white_check_mark:

Minimum allowed coverage is 65%

Generated by :monkey: cobertura-action against fc7890bf85d832d2a036308dc72d57519b4ac394

github-actions[bot] commented 1 year ago

Integration Tests Coverage Report

File Coverage Lines Branches
All files 72% 76% 68% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.MergedFullShadowSyncRequest 83% 91% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalUpdateSyncRequest 78% 83% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.Direction 83% 91% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalDeleteSyncRequest 54% 58% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.FullShadowSyncRequest 71% 79% 62% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudDeleteSyncRequest 52% 54% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.DirectionWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.OverwriteCloudShadowRequest 73% 71% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.BaseSyncRequest 52% 57% 46% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.OverwriteLocalShadowRequest 29% 29% 0% :x:
com.aws.greengrass.shadowmanager.sync.model.CloudUpdateSyncRequest 62% 60% 64% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDatabase 60% 60% 60% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManager 90% 91% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDAOImpl 81% 87% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.AuthorizationHandlerWrapper 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.PubSubIntegrator 25% 22% 29% :x:
com.aws.greengrass.shadowmanager.ShadowManager$1 14% 14% 0% :x:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractDeleteThingShadowOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractListNamedShadowsForThingOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractUpdateThingShadowOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractGetThingShadowOperationHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonMerger 66% 73% 59% :white_check_mark:
com.aws.greengrass.shadowmanager.util.SyncNodeMerger 76% 88% 64% :white_check_mark:
com.aws.greengrass.shadowmanager.util.DataOwner 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.ShadowWriteSynchronizeHelper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.Validator 67% 68% 65% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonUtil 83% 90% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.BaseRequestHandler 77% 77% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowRequestHandler 75% 74% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.PubSubClientWrapper 82% 82% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.IpcRateLimiter 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowIPCHandler 55% 55% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter 94% 100% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowRequestHandler 86% 72% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter$1 75% 100% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowRequestHandler 74% 77% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowIPCHandler 53% 53% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowIPCHandler 95% 95% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.ListNamedShadowsForThingIPCHandler 61% 55% 67% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.Operation 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.PubSubRequest 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.PeriodicSyncStrategy 71% 92% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.SyncStrategyFactory 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.BaseSyncStrategy 78% 87% 70% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.RealTimeSyncStrategy 83% 90% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientFactory 77% 93% 61% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestBlockingQueue 61% 70% 52% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.SyncHandler 82% 93% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestMerger 60% 67% 52% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.CloudDataClient 49% 45% 53% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ShadowSyncConfiguration 71% 81% 61% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ThingShadowSyncConfiguration 76% 78% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.ShadowDocSizeConfiguration 61% 73% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.RateLimitsConfiguration 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.configuration.ComponentConfiguration 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.StrategyType 55% 69% 40% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.Strategy 90% 100% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ResponseMessageBuilder 91% 91% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowDocument 89% 91% 87% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowStateMetadata 83% 88% 78% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ErrorMessage 97% 94% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowRequest 88% 77% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowState 96% 98% 94% :white_check_mark:
com.aws.greengrass.shadowmanager.model.LogEvents 100% 100% 0% :white_check_mark:

Minimum allowed coverage is 45%

Generated by :monkey: cobertura-action against fc7890bf85d832d2a036308dc72d57519b4ac394

MikeDombo commented 1 year ago

Check on the UAT failure as well. https://github.com/aws-greengrass/aws-greengrass-shadow-manager/actions/runs/6475607422/job/17582906225?pr=196

jcosentino11 commented 1 year ago

Check on the UAT failure as well. https://github.com/aws-greengrass/aws-greengrass-shadow-manager/actions/runs/6475607422/job/17582906225?pr=196

yep, still digging in

jcosentino11 commented 1 year ago

Check on the UAT failure as well. https://github.com/aws-greengrass/aws-greengrass-shadow-manager/actions/runs/6475607422/job/17582906225?pr=196

Looks unrelated to this PR

Given my device is registered as a Thing....................................passed
--
And my device is running Greengrass.........................................passed
And I start an assertion server.............................................passed
Given I create a Greengrass deployment with components......................failed
And I deploy the Greengrass deployment configuration........................skipped
Then the Greengrass deployment is COMPLETED on the device after 3 minutes...skipped
Then I verify the aws.greengrass.ShadowManager component is RUNNING using the greengrass-cli.skipped
When I install the component ShadowReactiveComponentPing from local store with configuration.skipped
And I install the component ShadowComponentPing from local store with configuration.skipped
Then the local Greengrass deployment is SUCCEEDED on the device after 120 seconds.skipped
When I install the component ShadowComponentPong from local store with configuration.skipped
Then the local Greengrass deployment is SUCCEEDED on the device after 120 seconds.skipped
 
StackTrace:
java.lang.NoSuchMethodError: 'software.amazon.awssdk.services.s3.internal.endpoints.S3EndpointResolverContext$Builder software.amazon.awssdk.services.s3.internal.endpoints.S3EndpointResolverContext$Builder.endpointOverridden(boolean)'
at software.amazon.awssdk.services.s3.internal.handlers.EndpointAddressInterceptor.modifyHttpRequest(EndpointAddressInterceptor.java:47)
at software.amazon.awssdk.core.interceptor.ExecutionInterceptorChain.modifyHttpRequestAndHttpContent(ExecutionInterceptorChain.java:90)
at software.amazon.awssdk.core.internal.handler.BaseClientHandler.runModifyHttpRequestAndHttpContentInterceptors(BaseClientHandler.java:157)
at software.amazon.awssdk.core.internal.handler.BaseClientHandler.finalizeSdkHttpFullRequest(BaseClientHandler.java:83)
at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.doExecute(BaseSyncClientHandler.java:151)
at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.lambda$execute$1(BaseSyncClientHandler.java:82)
at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.measureApiCallSuccess(BaseSyncClientHandler.java:179)
at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.execute(BaseSyncClientHandler.java:76)
at software.amazon.awssdk.core.client.handler.SdkSyncClientHandler.execute(SdkSyncClientHandler.java:45)
at software.amazon.awssdk.awscore.client.handler.AwsSyncClientHandler.execute(AwsSyncClientHandler.java:56)
at software.amazon.awssdk.services.s3.DefaultS3Client.headBucket(DefaultS3Client.java:5262)
at com.aws.greengrass.testing.resources.s3.S3Lifecycle.bucketExists(S3Lifecycle.java:38)
at com.aws.greengrass.testing.modules.AWSResourcesCleanupModule$CleanupInterceptor.invoke(AWSResourcesCleanupModule.java:89)
at com.aws.greengrass.testing.component.RecipeComponentPreparationService.lambda$getOrCreateBucket$1(RecipeComponentPreparationService.java:130)
at java.base/java.util.Optional.orElseGet(Optional.java:369)
at com.aws.greengrass.testing.component.RecipeComponentPreparationService.getOrCreateBucket(RecipeComponentPreparationService.java:128)
at com.aws.greengrass.testing.component.RecipeComponentPreparationService.prepare(RecipeComponentPreparationService.java:187)
at com.aws.greengrass.testing.component.CompositeComponentPreparationService.lambda$prepare$0(CompositeComponentPreparationService.java:31)
at java.base/java.util.Optional.map(Optional.java:265)
at com.aws.greengrass.testing.component.CompositeComponentPreparationService.prepare(CompositeComponentPreparationService.java:29)
at com.aws.greengrass.testing.features.DeploymentSteps.lambda$parseComponentNamesAndPrepare$1(DeploymentSteps.java:353)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at com.aws.greengrass.testing.features.DeploymentSteps.parseComponentNamesAndPrepare(DeploymentSteps.java:330)
at com.aws.greengrass.testing.features.DeploymentSteps.createDeployment(DeploymentSteps.java:123)
at ✽.I create a Greengrass deployment with components(classpath:greengrass/features/shadow-1.feature:10)
MikeDombo commented 1 year ago

https://github.com/aws-greengrass/aws-greengrass-testing/pull/226 Looks like SDK version mismatch due to this update.

jcosentino11 commented 1 year ago

May be related to https://github.com/aws-greengrass/aws-greengrass-testing/pull/226, UAT runs after this have been failing