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

Support moto by allowing non-awaitable content #1007

Closed
wants to merge 3 commits into from

Conversation

akshara08
Copy link

@akshara08 akshara08 commented Apr 18, 2023

Description of Change

PR adds a patch to endpoint.convert_to_response_dict such that it can work with moto while not changing the expected behavior of the method. Tests are added

Issue (#979)

The primary problem with moto integration is this method. There are 2 main issues with that method

  1. It expects raw to have raw_headers just to convert the headers to lowercase.
  2. Moto passes MockRawResponse which is not awaitable causing the breaks with lines containing await.

How to reproduce the error

# test_response.py

@pytest.fixture
def aws_credentials():
    """Mocked AWS Credentials for moto."""
    os.environ["AWS_ACCESS_KEY_ID"] = "testing"
    os.environ["AWS_SECRET_ACCESS_KEY"] = "testing"
    os.environ["AWS_SECURITY_TOKEN"] = "testing"
    os.environ["AWS_SESSION_TOKEN"] = "testing"


@pytest.fixture
def s3(aws_credentials):
    with mock_s3():
        conn = boto3.resource("s3")
        conn.create_bucket(Bucket='testbucket')
        yield conn


def test_moto(s3):
    for i in range(3):
        s3.Bucket('testbucket').put_object(
            Key=f'glob_{i}.txt', Body=f"test glob file {i}"
        )
    path = 's3://testbucket/glob_*.txt'
    fs = s3fs.S3FileSystem()
    files = fs.glob(path) # Fails 
    assert len(files) == 3

Resolution

It's a two part solution

  1. The first part will be a addressed on moto repo by creating raw_headers (PR)
  2. The second part of the problem can be addressed in this repo by only awaiting for awaitable objects.

Assumptions

  1. Assigning non-awaitable objects to response_dict['body'] without await is not expected to break any current behavior

Alternative methods considered

I tried consolidating all the changes in just one repo (see: #1004) but it's not possible

Checklist for All Submissions

  • I have added change info to CHANGES.rst
  • If this is resolving an issue (needed so future developers can determine if change is still necessary and under what conditions) (can be provided via link to issue with these details):
    • Detailed description of issue
    • Alternative methods considered (if any)
    • How issue is being resolved
    • How issue can be reproduced
  • (NA) If this is providing a new feature (can be provided via link to issue with these details):
    • Detailed description of new feature
    • Why needed
    • Alternatives methods considered (if any)

Checklist when updating botocore and/or aiohttp versions

  • I have read and followed CONTRIBUTING.rst
  • I have updated test_patches.py where/if appropriate (also check if no changes necessary)
  • I have ensured that the awscli/boto3 versions match the updated botocore version

@akshara08
Copy link
Author

@thehesiod This is a much smaller PR. This needs to be merged for this PR to work completely.

Comment on lines 57 to 60
if isawaitable(http_response.content):
response_dict['body'] = await http_response.content
else:
response_dict['body'] = http_response.content
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated!

@akshara08
Copy link
Author

Also, I just came across this comment from you. 🤕 Is this all fixable someplace else?

@akshara08
Copy link
Author

Alright don't bother merging this: See comment. I'm gonna peace out for a bit and think about this

@akshara08 akshara08 closed this Apr 18, 2023
@akshara08 akshara08 deleted the moto_support branch April 18, 2023 23:30
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

Successfully merging this pull request may close these issues.

2 participants