Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: eliminate fragility of code that processes retrieved results from potentially processing results in the wrong order #227

Open
techfg opened this issue Sep 26, 2024 · 0 comments

Comments

@techfg
Copy link

techfg commented Sep 26, 2024

During a retrieve, the following occurs:

  1. Get Retrieve Plans - This is strongly typed to only capture the skuidMetadataService and skuidCloudDataService plans in a response (any other plan that may exist will be ignored)
  2. Execute Retrieve Plans - This is strongly typed to always process the skuidMetadataService plan first, then the skuidCloudDataService plan 2nd and it returns an array of results in that order
  3. Write results to disk - This is based on the order of the results from step 2 and when a file exists in both results, merges 2nd results on top of the first result.

Based on this line of code, it would appear that a skuidMetadataService plan is always expected to be returned and optionally a skuidCloudDataService plan. However, based on this line of code, that may not be the case and it may be OK/expected that skuidMetadataService is not returned in some situations. Reading the code exactly as written, possibly in the scenario that --since was specified, its expected that skuidMetadataService will always exist but if --since is not specified, its possible that skuidMetadataService will NOT exist. Although this would not make much sense I don't think.

All things considered, assuming that a skuidMetadataService plan is always expected (even if it doesn't contain any metadata inside), if the code in step 2 is ever modified to return results in the slice in a different order and step 3 not updated accordingly, results written to disk may not be expected because of the way CombineJSON works as merging "file 1" on top of "file 2" could have a different outcome as merging "file 2" on top of "file 1" (see #197).

Given the existing code, it would appear that the following is expected:

  1. Get Retrieve Plans - Always returns a skuidMetadataService plan in ALL situations even if there are no entities in Metadata types and optionally skuidCloudDataService (see bug: retrieve indicates success but results in an empty targetDirectory without any warning or error #225 & bug: retrieve panics when skuidMetadataService is not present in response #226)
  2. Execute Retrieve Plans - Always processes the skuidMetadataService first followed by skuidCloudDataService if applicable
  3. Write Results - Always process skuidMetadataService first followed by skuidCloudDataService if it exists

Is this expectation correct? Are there any situations where a skuidCloudDataService would be returned but not a skuidMetadataService?

The code should be updated to explicitly process all responses if the order of processing is relevant for ensuring correct final result. Beyond the unpredictable/unreliable nature of v0.6.7 of the CLI and the server apis in general, there is too much risk that someone (even someone at Skuid) could unknowingly change the order in Step 2 and not even be aware of the fact that order is critical (took me 3 months of work on/off to even figure this out and still not sure I'm correct).

techfg added a commit to techfg/skuid-cli that referenced this issue Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant