fix: Writing to bytes TextIO stream in StreamWriter causing AttributeError on buffer member

[sbchisholm]

Which issue(s) does this change fix?

#6033

Why is this change necessary?

Fixes the bug raise in #6033 where the instance of the stream member in StreamWriter does not have the buffer attribute.

How does it address the issue?

Rather than using the buffer attribute to write bytes, the bytes are converted to a string first with decode and then written with the write method (which accepts strings)

What side effects does this change have?

Unsure.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[wolverian]

Looking at the users of this function, for example

aws-sam-cli/samcli/local/docker/container.py

Lines 476 to 478 in 8126aaa

	output_data = output_data.decode("utf-8").replace("\r", os.linesep).encode("utf-8")
	if isinstance(output_stream, StreamWriter):
	output_stream.write_bytes(output_data)

and

aws-sam-cli/samcli/commands/remote/remote_invoke_context.py

Line 276 in 8126aaa

self._stream_writer.write_bytes(cast(str, remote_invoke_response.response).encode())

would it make sense to remove this function and change the calls to use write_str instead?

[sbchisholm]

Looking at the users of this function, for example

aws-sam-cli/samcli/local/docker/container.py

Lines 476 to 478 in 8126aaa

	output_data = output_data.decode("utf-8").replace("\r", os.linesep).encode("utf-8")
	if isinstance(output_stream, StreamWriter):
	output_stream.write_bytes(output_data)

and

aws-sam-cli/samcli/commands/remote/remote_invoke_context.py

Line 276 in 8126aaa

self._stream_writer.write_bytes(cast(str, remote_invoke_response.response).encode())

would it make sense to remove this function and change the calls to use write_str instead?

If that's true, then that would make sense but it looks to also be used in here:

aws-sam-cli/samcli/local/docker/container.py

Lines 367 to 368 in 8126aaa

	elif isinstance(response, bytes):
	stdout.write_bytes(response)

[wolverian]

Sure, but does that make sense? In that case, if this PR is merged, the bytes are decoded to a string anyway, it's just hidden from the caller and could surprise them if they assumed the bytes wouldn't be interpreted in any specific encoding.

Rather, in my opinion, either this abstraction should offer a way to write raw bytes with no interpretation (decoding) or it should only offer a way to write strings.

Hiding these bytes to string conversions will shoot the user in the foot eventually.

[sbchisholm]

Sure, but does that make sense? In that case, if this PR is merged, the bytes are decoded to a string anyway, it's just hidden from the caller and could surprise them if they assumed the bytes wouldn't be interpreted in any specific encoding.

Rather, in my opinion, either this abstraction should offer a way to write raw bytes with no interpretation (decoding) or it should only offer a way to write strings.

Hiding these bytes to string conversions will shoot the user in the foot eventually.

That's a great point, I'll update the PR.

[sriram-mv]

I'm adding both @jfuss and @mildaniel as reviewers given their previous experience in bytes/str operation.

My take is that we are explicitly changing behavior from bytes to str and there could be side-effects. There was an explicit PR to decode - replace carriage returns - reencode. That behavior seems to be removed at this point.

[wolverian]

My take is that we are explicitly changing behavior from bytes to str and there could be side-effects. There was an explicit PR to decode - replace carriage returns - reencode. That behavior seems to be removed at this point.

It might be a good idea to bisect which commit triggered this bug. That might help with figuring out what the right thing to do here is. FWIW, I think this PR looks logical, but I'm unfamiliar with this codebase and its history.

This is fairly high in priority in my opinion as I simply can't use the latest version SAM CLI right now due to this bug.

[mildaniel]

I did some digging here and it seems as though the issue is that the StreamWriter class expects the stream to be of type TextIO, but there are times where an io.StringIO is passed in https://github.com/aws/aws-sam-cli/blob/develop/samcli/local/lambda_service/local_lambda_invoke_service.py#L165.

io.StringIO is not of type TextIO and does not have a buffer property that StreamWriter expects of a TextIO object. I'm not sure why this wasn't picked up by any type checking, but in any case, causes this exception whenever it's called with an io.StringIO stream. Changing the instantiators of the StreamWriter class to all use TextIO would require pretty significant refactoring since those instances also use methods and properties unique to io.StringIO.

Although this PR is more of a solution to the symptom rather than root cause, I'm inclined to go with it so I don't really see any negatives here and it allows us to patch an important issue. Another solution is to update the type in the StreamWriter class to be a union of StreamWriter, io.TextIO and io.TextIOWrapper and handle each one in the implementation of the write method.

@sriram-mv what are your thoughts here?