getmoto / moto

A library that allows you to easily mock out tests based on AWS infrastructure.
http://docs.getmoto.org/en/latest/
Apache License 2.0
7.65k stars 2.05k forks source link

TypeError when using S3 Bucket.objects.filter(Marker=...) #7521

Closed jvtm closed 7 months ago

jvtm commented 7 months ago

When calling objects.filter() with an existing key as Marker:

bucket.objects.filter(Marker=marker)

Moto 5.0.x crashes on TypeError (trying to order str vs None):

motobug/test_motobug.py:65: in batch_from
    for obj in bucket.objects.filter(Marker=marker):

...

 ../../external/moto/moto/s3/responses.py:291: in bucket_response
    response = self._bucket_response(request, full_url)
../../external/moto/moto/s3/responses.py:330: in _bucket_response
    return self._bucket_response_get(bucket_name, querystring) 
../../external/moto/moto/s3/responses.py:698: in _bucket_response_get
    ) = self.backend.list_objects(
../../external/moto/moto/s3/models.py:2580: in list_objects
    key_results = self._get_results_from_token(key_results, limit)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <moto.s3.models.S3Backend object at 0x7fbcfdbb46a0>
result_keys = [<moto.s3.models.FakeKey object at 0x7fbcfdbb47f0>, <moto.s3.models.FakeKey object at 0x7fbcfdc81360>, ...]
token = None

    def _get_results_from_token(self, result_keys: Any, token: Any) -> Any:
        continuation_index = 0
        for key in result_keys:
>           if (key.name if isinstance(key, FakeKey) else key) > token:
E           TypeError: '>' not supported between instances of 'str' and 'NoneType'

Seeing this on 5.0.3 and also latest 5.0.4.dev / ade1001a6928.

This used to work with 4.x series.

Current versions of relevant libraries (all latest when writing this):

$ poetry run pip freeze | grep -E "^(boto3|botocore|moto)="
boto3==1.34.69
botocore==1.34.69
moto==5.0.3

Boto3 docs: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3/bucket/objects.html#filter


"Short" pytest code for reproducing (a bit of boilerplate for bucket setup, AWS env vars, moto 4.x vs 5.x compatibility...). 5.x fails on second call which uses a marker that is an existing key from the last result set.

from __future__ import annotations

import logging
import uuid
from typing import TYPE_CHECKING, Final

import boto3
import moto
import pytest

if TYPE_CHECKING:
    from collections.abc import Generator

    from mypy_boto3_s3.service_resource import Bucket, ObjectSummary

DEFAULT_REGION: Final = "eu-west-1"
BUCKET_NAME: Final = "test-bucket"

@pytest.fixture()
def moto_patch(monkeypatch: pytest.MonkeyPatch)  -> Generator[pytest.MonkeyPatch, None, None]:
    monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testmoto")
    monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testmoto")
    monkeypatch.setenv("AWS_SECURITY_TOKEN", "testmoto")
    monkeypatch.setenv("AWS_SESSION_TOKEN", "testmoto")
    monkeypatch.setenv("AWS_DEFAULT_REGION", DEFAULT_REGION)
    monkeypatch.setenv("AWS_CONFIG_FILE", "/dev/null")
    monkeypatch.setenv("BOTO_CONFIG", "/dev/null")
    monkeypatch.delenv("AWS_PROFILE", raising=False)

    logging.info("moto %r", moto.__version__)
    if hasattr(moto, "mock_aws"):  # moto 5.x
        with moto.mock_aws():
            yield monkeypatch
    elif hasattr(moto, "mock_s3"):  # moto 4.x
        with moto.mock_s3():
            yield monkeypatch
    else:
        raise AttributeError("Unknown moto version")

@pytest.fixture()
def bucket(moto_patch: pytest.MonkeyPatch) -> Bucket:
    s3 = boto3.resource("s3")
    return s3.create_bucket(
        Bucket=BUCKET_NAME,
        CreateBucketConfiguration={
            "LocationConstraint": DEFAULT_REGION,
        },
    )

def test_s3_filter_with_marker(bucket: Bucket) -> None:
    def batch_from(marker: str, *, batch_size: int) -> list[ObjectSummary]:
        first: str | None = None
        last: str | None = None
        ret: list[ObjectSummary] = []

        logging.info("PRE: marker: %r, batch_size: %r", marker, batch_size)

        # page_size() doesn't affect the bug; real code could have also sane
        # defaults and limits
        # for obj in bucket.objects.page_size(batch_size).filter(Marker=marker):
        for obj in bucket.objects.filter(Marker=marker):
            if first is None:
                first = obj.key
            last = obj.key
            ret.append(obj)
            if len(ret) >= batch_size:
                break

        logging.info("POST marker: %r, count: %d, first: %r, last: %r", marker, len(ret), first, last)
        return ret

    # create some objects
    xid = uuid.uuid4()
    keys = [f"{xid}/{i:016d}" for i in range(37)]
    for key in keys:
        bucket.put_object(Body=f"hello {key}".encode(), Key=key)

    # all() gives out all objects
    assert [x.key for x in bucket.objects.all()] == keys
    # filter() with Prefix, without Marker works
    assert [x.key for x in bucket.objects.filter(Prefix=f"{xid}/")] == keys

    # batches using filter(Merker=...); moto 5.0.3 crashes on the second round
    # > if (key.name if isinstance(key, FakeKey) else key) > token:
    # E TypeError: '>' not supported between instances of 'str' and 'NoneType'
    marker = ""
    batch_size = 10
    collect: list[ObjectSummary] = []

    while batch := batch_from(marker, batch_size=batch_size):
        collect.extend(batch)
        marker = batch[-1].key

    assert [x.key for x in collect] == keys

This is a bit artificial but not far from some real code using bigger batches / pagination / markers.

bblommers commented 7 months ago

Hi @jvtm, thank you for raising this and for adding a repro case! That functionality was indeed broken by accident - we didn't have any tests for that usecase apparently.

I'm opening a PR with a fix now.

jvtm commented 7 months ago

Hi @jvtm, thank you for raising this and for adding a repro case! That functionality was indeed broken by accident - we didn't have any tests for that usecase apparently.

I'm opening a PR with a fix now.

Just tried the PR out with tests having way larger amount of objects, and things seem to pass again. Nice.