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

Moto integration #1004

Closed
wants to merge 8 commits into from
Closed

Conversation

akshara08
Copy link

@akshara08 akshara08 commented Apr 5, 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. I also added tests to replicate the problem and fix the issue.

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. Remove the need for raw_headers and use the headers directly using botocore.utils.lowercase_dict

  1. Since raw_headers are necessary, I've created a check to use headers if the httpresponse came from moto
  2. Only await for awaitable objects. This check can be done using inspect.isawaitable

Assumptions

1. This patch was added only to convert to lowercase and this botocore util does the same
^Not valid anymore since raw_headers remain

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

Alternative methods considered

I considered added a raw_headers to MockRawResponse but it seemed unnecessary especially if the point to convert to lowercase based on the comment

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

‼️ Note ‼️

I've been getting botocore.exceptions.NoCredentialsError: Unable to locate credentials for some of the tests (5 to be exact). Ive got them even before I made changes, so I'm unsure if I'm breaking anything with this PR

@akshara08 akshara08 marked this pull request as draft April 5, 2023 22:58
Pipfile Outdated
@@ -18,6 +18,7 @@ pytest = "==6.2.4"
pytest-cov = "==2.11.1"
pytest-asyncio = "==0.14.0"
pytest-xdist = "==2.2.1"
s3fs = "==2023.3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this introduces a circular dependency, can't do it as it may later be incompatible with the version of aiobotocore this becomes

Copy link
Author

@akshara08 akshara08 Apr 6, 2023

Choose a reason for hiding this comment

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

Makes sense. Removed the dependency, used paginator for testing instead

'headers': HTTPHeaderDict(
{
k.decode('utf-8').lower(): v.decode('utf-8')
for k, v in http_response.raw.raw_headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's important we use raw_headers as the non-raw doesn't preserve casing of the value

Copy link
Author

Choose a reason for hiding this comment

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

Alright, the alternative is kind of an ugly patch. Let me know what ya think

tests/test_response.py Outdated Show resolved Hide resolved
@akshara08 akshara08 marked this pull request as ready for review April 6, 2023 05:25
@akshara08 akshara08 requested a review from thehesiod April 6, 2023 18:23
@akshara08
Copy link
Author

@thehesiod any update on this?

@thehesiod
Copy link
Collaborator

re NOTE in your description you need to run with -m moto to limit to moto marked tests, all other tests are only to be run with private key as moto didn't have support for them when tests were originally written (we should revisit these tests and mark what exactly is missing for the test).

@@ -1,5 +1,9 @@
Changes
-------
2.5.1 (2023-04-05)
^^^^^^^^^^^^^^^^^^
* integrate moto
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is more of add support for moto in-place mock methods

@@ -13,6 +14,8 @@
logger,
)
from botocore.hooks import first_non_none_response
from botocore.utils import lowercase_dict
from moto.core.botocore_stubber import MockRawResponse
Copy link
Collaborator

Choose a reason for hiding this comment

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

moto is not a dependency of this library, so you can't do this. I think this is really a bug in moto, it needs support for aiobotocore. If you were to do this it would need to be a try/except ImportError. Another option would be for aiobotocore to expose some moto helpers that correctly patch the response. I'd do that instead as it will be a lot cleaner.

Copy link
Author

@akshara08 akshara08 Apr 18, 2023

Choose a reason for hiding this comment

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

moto is not a dependency of this library, so you can't do this.

Ack! I missed that!
I'm a little busy this week but I'll try to sneak in some time to dig into moto repo and see if there's a cleaner solution to this. I tried sending encoded raw_headers here and it broke a bunch of tests. Let me try that again and see

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries, thanks for working on this

@thehesiod
Copy link
Collaborator

sorry for delay, I think either moto needs to add async patches, or we ship like a moto_helpers.py which does the right thing.

@akshara08
Copy link
Author

Closing this to avoid piling on irrelevant commits. See #1007

@akshara08 akshara08 closed this Apr 18, 2023
@akshara08 akshara08 deleted the moto_integration branch April 18, 2023 21:14
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