Add support for list types and validator functions in GlobalsHelperFn type check

[TeeSeal]

Closes #2063

I tried extracting the function to troposhpere.validators.__init__.py as it made more sense to have it there, but its dependence on AWSHelperFn created a circular import with troposhpere.__init__.py. Fixing that would require picking apart the entire troposhpere.__init__.py file.

[TeeSeal]

Cannot reproduce linting failure locally and neither do I see how to fix it.

❯ npx pyright troposphere/__init__.py
No configuration file found.
pyproject.toml file found at /Users/alex/Developer/repos/troposphere.
Loading pyproject.toml file at /Users/alex/Developer/repos/troposphere/pyproject.toml
Auto-excluding **/node_modules
Auto-excluding **/__pycache__
Auto-excluding **/.*
venvPath not specified, so venv settings will be ignored.
stubPath /Users/alex/Developer/repos/troposphere/typings is not a valid directory.
Searching for source files
Found 1 source file
pyright 1.1.263
0 errors, 0 warnings, 0 informations 
Completed in 0.683sec

Running on macOS 12.4. Would appreciate some assistance with that.

[markpeek]

@TeeSeal Thank you for the PR and pointing out this issue. I pulled it down to take a look. There were a couple of issues that I noticed:

• make test had multiple failures since the tests are trying to match the exception signature
• Splitting out validate_types() into a function decouples it from some of the class info needed to formulate the exception
• Catching the exception from validate_types() makes it hard to suggest the expected_type
• The value from the validator function in serverless.py would need to be assigned back into the resultant object.

I have an alternative way to resolve this issue. Instead of having a separate GlobalsHelperFn, how about removing it and using AWSProperty which would handle that same core logic?

For example, please try this type of change and let me know if you think it would resolve the issue. And then we could make the same change across the remaining serverless.py Global* objects.

-class FunctionGlobals(GlobalsHelperFn):
+class FunctionGlobals(AWSProperty):

$ python
Python 3.9.12 (main, May  8 2022, 18:05:47)
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from troposphere.serverless import FunctionGlobals
>>> print(FunctionGlobals(MemorySize=3072).to_dict())
{'MemorySize': 3072}

[TeeSeal]

@markpeek yeah... totally forgot to run tests before submitting. My bad.

Thanks a lot for your suggestion. I was initially looking for a solution like that (i.e. tried inheriting from AWSBaseObject which failed) but my lack of familiarity with the codebase caused me to go awry and overcomplicate things.

I've applied your suggestion and updated the tests accordingly. This caused 3 potentially breaking changes:

1. GlobalsHelperFn is gone from serverless.py. It's not being used anywhere else in the library and I took it upon myself to remove it. I am quite sure it is not considered "public API" but need to still confirm if that's alright. Do let me know if I should add it back.

2. *Globals classes now raise different errors during validation.

   • Providing an invalid attribute (Globals(InvalidAttribute=None)) previously raised ValueError, now raises AttributeError
   • Providing an invalid value for one of the attributes (Globals(Function="invalid")) previously raised ValueError, now raises TypeError

3. *Globals classes no longer validate for required attributes in #__init__, but do so in #to_dict instead.

Now this works absolutely fine for my use case, but please do consider the above before merging.

[markpeek]

Thank you @TeeSeal for changing the implementation. To your points:

1. Agreed, I don't consider GlobalsHelperFn a public API and it can be removed.
2. Different exceptions is a bit of a breaking change but I think worth it for the benefit of consolidating the handling of property values. Not sure how many people actually catch these exceptions. I'll make a note in the release notes.
3. This isn't accurate as individual properties are validated at object creation. There is full object validation done later but none of the serverless Global* classes have a validation method. For example:

   >>> from troposphere.serverless import FunctionGlobals
   >>> FunctionGlobals(MemorySize=64)
   <class 'troposphere.serverless.FunctionGlobals'>: None.MemorySize function validator 'validate_memory_size' threw exception:
   Traceback (most recent call last):
   ...
   ValueError: Lambda Function memory size must be between 128 and 10240

This is pretty much good to go now. Unless you get to it before I do, the only thing to add is some positive and negative tests for the function and list [*] variants that are now supported by this change.

[TeeSeal]

@markpeek I don't think we're on the same page regarding point 3. What I meant to highlight is the check for the actual required flag in the definition of a property.

"RequiredProperty": (str, True),
#                          ^--- this right here

Given that the above was a real property defined for, say, FunctionGlobals:

• Previously simply calling FunctionGlobals() would throw ValueError: Required FunctionGlobals property 'RequiredProperty' not provided
• With the update this no longer happens but a similar behaviour can be achieved by doing FunctionGlobals().to_dict() instead, which throws ValueError: Resource RequiredProperty required in type <class 'troposphere.serverless.FunctionGlobals'>

However, when doing a double check now I found that all properties defined on *Globals are optional so this is of little to no relevance. Can just scratch it.

---

I've added the requested tests 👍

[markpeek]

Thank you for the PR and adding the tests.