Implement ReadStateBytes + WriteStateBytes #37440
Conversation
radeksimko
added
the
no-changelog-needed
radeksimko
force-pushed
the
radek/pss-read-write
branch
2 times, most recently
from
53e451b to
f185ebe
Compare
radeksimko
force-pushed
the
radek/pss-read-write
branch
from
f185ebe to
74fd3bc
Compare
* Fix nil pointer error * Add WIP test for ReadStateBytes * Move test mock to separate testing file * Update mock to send unexpected EOF when there's a problem returning data and it's not a true EOF * Add test case for when length != expected length * Add test for when trying to read state from a store type that doesn't exist * Change symbol names to lowercase * Add ability to force a diagnostic to be returned from `mockReadStateBytesClient`'s `Recv` method * Add test showing error diagnostics raised by the ReadStateBytes client are returned * Add missing header * Simplify mock by using an embedded type * Rename `mockOpts` to `mockReadStateBytesOpts` * Update existing tests to assert what arguments are passed to the RPC method call * Add mock WriteStateBytesClient which uses `go.uber.org/mock/gomock` to enable assertions about calls to Send * Add a test for WriteStateBytes that makes assertions about calls to the Send method * Update test case to explicitly test writing data smaller than the chunk size * Implement chunking in WriteStateBytes, add test case to assert expected chunking behaviour * Add generated mock for Provider_WriteStateBytesClient in protocol v6 * Update tests to use new `MockProvider_WriteStateBytesClient`, remove handwritten mock * Update code comments in test * Add tests for diagnostics and errors returned during WriteStateBytes * Add generated mock for Provider_ReadStateBytesClient in protocol v6, replace old mock * Add test case for grpc errors in ReadStateBytes, fix how error is returned * Typo in comment * Add missing warning test, rename some test cases
* Update Read/WriteStateBytes RPCs to match hashicorp/terraform-plugin-go#531 * Run `make protobuf` * Run `make generate` * Update use of `proto.ReadStateBytes_ResponseChunk` in tests * Fix how diagnostics are handled alongside EOF error, update ReadStateBytes test * More fixes - test setup was incorrect I think? I assume that a response would be returned full of zero-values when EOF is encountered.
| if err == io.EOF { | ||
| // End of stream, we're done | ||
| if chunk != nil { | ||
| // TODO: The EOF error could be just returned alongside the last chunk? | ||
| resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(chunk.Diagnostics)) | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
I was looking online for guidance about whether an EOF error could/should accompany read data or should come after the last piece of data, and multiple sources referred back to the io package's docs:
When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call. An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.
This makes me think that we're free to decide whether the EOF comes with or after the last response that contains read data. I'm purposefully avoiding the word chunk as a response doesn't necessarily contain a chunk/bytes of data.
However, I think given the other examples of streaming in TF core it may be easiest to follow the choice to have the EOF error be sent on its own and not alongside data. That would mean we do not expect any diagnostics to accompany the EOF. This matches conventions in the code base already, and avoids the differentiation between responses generally and responses that represent chunks.
How does that sound?
Target Release
1.14.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry