Skip to content

feat(parameters): transform = "auto" #133

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

Merged
merged 7 commits into from
Sep 2, 2020

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Aug 26, 2020

Issue #, if available: #136

When fetching multiple values from the parameter store or dynamodb often each record might have different encoding and not all have uniform encoding. eg:

key value
A json string
B base64 decoded image
C might be a plain string

Description of changes:

Add a new tranform type called "auto", which looks at the
key name to autodetect the encoding of the values in dynamodb or
the parameter store.

  • Keys ending with ".json" are assumed to be "json" string
  • Keys ending with ".binary" are assumed to be "binary" base64 decode
    strings
  • Other Keys are assumed to be plain text values.
key value
A.json ["one", "two"]
B.binary b'base64 decoded image'
C plain text string

Checklist

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

Add a new tranform type called "auto", which looks at the
key name to autodetect the encoding of the values in dynamodb or
the parameter store.

Keys ending with ".json" are assumed to be "json" string
Keys ending with ".binary" are assumed to be "binary" base64 decode
strings
@michaelbrewer michaelbrewer changed the title feat(parameters): transform = "auto" feat(parameters): transform = "auto" [DRAFT] Aug 26, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2020

Codecov Report

Merging #133 into develop will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #133   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines          793       806   +13     
  Branches        72        76    +4     
=========================================
+ Hits           793       806   +13     
Impacted Files Coverage Δ
aws_lambda_powertools/utilities/parameters/base.py 100.00% <100.00%> (ø)

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 b28c47e...01e991f. Read the comment docs.

@michaelbrewer michaelbrewer changed the title feat(parameters): transform = "auto" [DRAFT] feat(parameters): transform = "auto" [RFC] Aug 27, 2020
@heitorlessa heitorlessa requested a review from nmoutschen August 29, 2020 06:51
@michaelbrewer
Copy link
Contributor Author

@nmoutschen - if it is a matter of flexibility on the naming pattern of the keys, we could create patterns for them.

Another optional it giving the new user an option to set a customer converter that accepts "key" and "value". So you could provide an auto-formatter, a yaml one, xml etc.

Copy link
Contributor

@nmoutschen nmoutschen left a comment

Choose a reason for hiding this comment

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

Hey @michaelbrewer !

Thanks a lot for your contribution! 😄 I have one open question regarding the user experience for this feature: should we rename the key by dropping the extension?

It would also be good to parameterise the transforms just to have a single place where to add/change transforms to prevent errors in the future if we want to add new transforms. However, this aspect is good as-is for my point of view.

@michaelbrewer
Copy link
Contributor Author

@nmoutschen would this patch make this more flexibile?

diff --git a/aws_lambda_powertools/utilities/parameters/base.py b/aws_lambda_powertools/utilities/parameters/base.py
index e1ba9ac..1269432 100644
--- a/aws_lambda_powertools/utilities/parameters/base.py
+++ b/aws_lambda_powertools/utilities/parameters/base.py
@@ -15,6 +15,9 @@ DEFAULT_MAX_AGE_SECS = 5
 ExpirableValue = namedtuple("ExpirableValue", ["value", "ttl"])
 # These providers will be dynamically initialized on first use of the helper functions
 DEFAULT_PROVIDERS = {}
+TRANSFORM_METHOD_JSON = "json"
+TRANSFORM_METHOD_BINARY = "binary"
+SUPPORTED_TRANSFORM_METHODS = [TRANSFORM_METHOD_JSON, TRANSFORM_METHOD_BINARY]
 
 
 class BaseProvider(ABC):
@@ -194,12 +197,10 @@ def get_transform_method(key: str, transform: str) -> Optional[str]:
     if transform != "auto":
         return transform
 
-    if key.endswith(".json"):
-        return "json"
-    elif key.endswith(".binary"):
-        return "binary"
-    else:
-        return None
+    for transform_method in SUPPORTED_TRANSFORM_METHODS:
+        if key.endswith("." + transform_method):
+            return transform_method
+    return None
 
 
 def transform_value(value: str, transform: str, raise_on_transform_error: bool = True) -> Union[dict, bytes, None]:
@@ -223,9 +224,9 @@ def transform_value(value: str, transform: str, raise_on_transform_error: bool =
     """
 
     try:
-        if transform == "json":
+        if transform == TRANSFORM_METHOD_JSON:
             return json.loads(value)
-        elif transform == "binary":
+        elif transform == TRANSFORM_METHOD_BINARY:
             return base64.b64decode(value)
         else:
             raise ValueError(f"Invalid transform type '{transform}'")

Changes:
* base.py - Add `SUPPORTED_TRANSFORM_METHODS` as a list of supported
  transform methods and add constants for the currently supported
  methods
* base.py - Update `get_transform_method` to use
  `SUPPORTED_TRANSFORM_METHODS` to check for supported extensions
Changes:
* Make `transform` optional in `get_transform_method`
* Update tests to include this use case
@michaelbrewer michaelbrewer changed the title feat(parameters): transform = "auto" [RFC] feat(parameters): transform = "auto" Sep 1, 2020
Copy link
Contributor

@nmoutschen nmoutschen left a comment

Choose a reason for hiding this comment

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

LGTM!

@to-mc to-mc self-requested a review September 2, 2020 07:11
@to-mc to-mc merged commit 9c09099 into aws-powertools:develop Sep 2, 2020
@michaelbrewer michaelbrewer deleted the feat-parameter-auto branch September 2, 2020 13:42
@heitorlessa heitorlessa added area/utilities feature New feature or functionality labels Sep 3, 2020
heitorlessa referenced this pull request in heitorlessa/aws-lambda-powertools-python Sep 11, 2020
* develop: (57 commits)
  chore: bump version to 1.5.0 (#158)
  chore(batch): Housekeeping for recent changes (#157)
  docs: address readability feedbacks
  chore: add debug logging for sqs batch processing
  chore: remove middlewares module, moving decorator functionality to base and sqs
  docs: add detail to batch processing
  fix: throw exception by default if messages processing fails
  chore: add test for sqs_batch_processor interface
  fix: add sqs_batch_processor as its own method
  docs: simplify documentation more SQS specific focus Update for sqs_batch_processor interface
  chore: add sqs_batch_processor decorator to simplify interface
  feat(parameters): transform = "auto" (#133)
  chore: fix typos, docstrings and type hints (#154)
  chore: tiny changes for readability
  fix: ensure debug log event has latest ctx
  docs: rephrase the wording to make it more clear
  docs: refactor example; improve docs about creating your own processor
  refactor: remove references to BaseProcessor. Left BasePartialProcessor
  docs: add newly created Slack Channel
  fix: update image with correct sample
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants