fix(parameters): appconfig transform and return types

[ran-isenberg]

Issue #, if available::

Description of changes:

Fix return type and handling transform value as str instead of bytes

Checklist

• Meet tenets criteria
• Update tests
• Update docs
• PR title follows conventional commit semantics

Breaking change checklist

No breaking changes. Introduces no new exception handling and would fix batch handling of more than 10 records.

No RFC:

• Migration process documented
• Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[heitorlessa]

Thanks a lot Ran! I'll look into it tomorrow morning

[heitorlessa]

Thanks a lot for the quick fix, Ran!

We will need to also update the base _get from BaseProvider to accept multiple types or else this will make MyPy fail with invalid override, and more importantly get_app_config also has the incorrect return type (bytes is missing).

I'm gonna update the list of papercuts to ensure we run MyPy at CI to catch these

[heitorlessa]

Bringing the discussion we're having on Slack so we can track the history afterwards @whardier, and to make sure the problem is well understood so we don't accidentally create another issue when fixing this one.

Based on your transform issue report (GitHub issue is missing), we seem to have two issues not one: Transform fails with JSON values (str instead of bytes), and _get return type expects str not Union[str, bytes] - please let me know if I misunderstood it.

Transform

Problem. By default AppConfig _get method returns bytes not str as the interfaces suggest. get_app_config public method however correctly indicates it could return bytes Union[str, list, dict, bytes].

Fix. We need to update transform_value function signature to also accept bytes value and handle it accordingly (avoid double decoding if it's bytes) - This function isn't documented despite being public (it shouldn't, oversight during the review), so it will not be a breaking change either way.

We don't need to touch AppConfig implementation besides return types; see Return types section.

Function signature change

• Current type annotation: def transform_value(value: str, transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:
• Suggested: def transform_value(value: Union[str, bytes], transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:

Handling bytes

This is the portion of the code that needs changing. We will need to decode bytes to str if the transform is JSON, and if the transform is Binary and the value received is already in bytes, there's no need to base64 decode it; simply return it.

def transform_value(value: str, transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:
    try:
        if transform == TRANSFORM_METHOD_JSON:
            return json.loads(value)
        elif transform == TRANSFORM_METHOD_BINARY:
            return base64.b64decode(value)
        else:
            raise ValueError(f"Invalid transform type '{transform}'")

    except Exception as exc:
        if raise_on_transform_error:
            raise TransformParameterError(str(exc))
        return None

Return types

Problem. AppConfig implementation of _get interface from BaseProvider uses a different return type which creates subtle bugs like these. Not surprisingly in the public documentation, we provide an example and explicitly call out get_app_config return type as bytes.

Fix. We will need _get to also return bytes.

• Current type annotation: def transform_value(value: str, transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:
• Suggested: def transform_value(value: Union[str, bytes], transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:

Overall, there are way more issues identified by MyPy around types which would require a cast and function overload, where we should do a proper solution as a whole. For now, let's just fix the transform function to handle bytes and the return type annotations.

[codecov-commenter]

Codecov Report

Merging #877 (6d00b06) into develop (0348bd6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #877      +/-   ##
===========================================
- Coverage    99.90%   99.88%   -0.02%     
===========================================
  Files          118      118              
  Lines         5126     5129       +3     
  Branches       571      573       +2     
===========================================
+ Hits          5121     5123       +2     
  Misses           2        2              
- Partials         3        4       +1     

Impacted Files	Coverage Δ
aws_lambda_powertools/metrics/metric.py	100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/parameters/base.py	100.00% <100.00%> (ø)
.../utilities/data_classes/cognito_user_pool_event.py	99.73% <0.00%> (-0.27%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0348bd6...6d00b06. Read the comment docs.

[heitorlessa]

Leaving other issues for posterity as there could be some other subtle bugs

---

aws_lambda_powertools/utilities/parameters/base.py: note: In member "get_multiple" of class "BaseProvider":
aws_lambda_powertools/utilities/parameters/base.py:158:64: error: Incompatible types in assignment (expression has type "Dict[str, str]", variable has type "Dict[str, Union[str, bytes, Dict[Any, Any], None]]")  [assignment]
                values: Dict[str, Union[str, bytes, dict, None]] = self._get_multiple(path, **sdk_options)
                                                                   ^
aws_lambda_powertools/utilities/parameters/base.py:158:64: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
aws_lambda_powertools/utilities/parameters/base.py:158:64: note: Consider using "Mapping" instead, which is covariant in the value type
aws_lambda_powertools/utilities/parameters/base.py:164:13: error: Incompatible types in assignment (expression has type "str", variable has type "Tuple[str, Optional[str]]")  [assignment]
                for (key, value) in values.items():
                ^
aws_lambda_powertools/utilities/parameters/base.py:165:51: error: Argument 1 to "get_transform_method" has incompatible type "Tuple[str, Optional[str]]"; expected "str"  [arg-type]
                    _transform = get_transform_method(key, transform)
                                                      ^
aws_lambda_powertools/utilities/parameters/base.py:169:24: error: Invalid index type "Tuple[str, Optional[str]]" for "Dict[str, Union[str, bytes, Dict[Any, Any], None]]"; expected type "str"  [index]
                    values[key] = transform_value(value, _transform, raise_on_transform_error)
                           ^
aws_lambda_powertools/utilities/parameters/base.py:169:47: error: Argument 1 to "transform_value" has incompatible type "Union[str, bytes, Dict[Any, Any], None]"; expected "str"  [arg-type]
                    values[key] = transform_value(value, _transform, raise_on_transform_error)
                                                  ^
aws_lambda_powertools/utilities/parameters/base.py:173:16: error: Incompatible return value type (got "Dict[str, Union[str, bytes, Dict[Any, Any], None]]", expected
"Union[Dict[str, str], Dict[str, Dict[Any, Any]], Dict[str, bytes]]")  [return-value]
            return values
                   ^
aws_lambda_powertools/utilities/parameters/ssm.py: note: In member "get" of class "SSMProvider":
aws_lambda_powertools/utilities/parameters/ssm.py:90:5: error: Signature of "get" incompatible with supertype "BaseProvider"  [override]
        def get(
        ^
aws_lambda_powertools/utilities/parameters/ssm.py:131:16: error: Incompatible return value type (got "Union[str, List[Any], Dict[Any, Any], bytes, None]", expected "Union[str, List[Any], Dict[Any, Any], bytes]")  [return-value]
            return super().get(name, max_age, transform, force_fetch, **sdk_options)
                   ^