Skip to content

feat: improve internal typings in manifest#278

Open
IzaakGough wants to merge 12 commits into
mainfrom
@invertase/improve-internal-typings-in-manifest
Open

feat: improve internal typings in manifest#278
IzaakGough wants to merge 12 commits into
mainfrom
@invertase/improve-internal-typings-in-manifest

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Tighten the internal manifest typing so manifest spec generation no longer falls back to _typing.Any.
  • Replace overly broad container types with explicit manifest value aliases and sequence/mapping handling that still accepts the runtime inputs the manifest code serializes today.

Problem/Root Cause

Issue #31 called out that the manifest internals still relied on _typing.Any, which left the manifest serialization path loosely typed even though it operates on a constrained set of values. The previous implementation used dict[str, _typing.Any], untyped helper inputs, and a narrow list check in _object_to_spec, so type information was lost across manifest parameter conversion and dataclass serialization.

Solution/Changes

This change introduces explicit aliases for supported manifest parameter types and manifest spec values, then threads those types through the manifest conversion helpers. The serialization path now distinguishes dataclass instances, mappings, and non-string sequences, which preserves support for inputs such as tuples while keeping the output normalized to manifest-compatible lists and dicts. It also updates the shared _params registry type so stored params are typed as Param or SecretParam instead of a generic Expression.

Testing

  • Verified existing CI checks passed on the PR, including format, lint, docs, Analyze, CodeQL, and test (3.10) / test (3.12).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves type safety in manifest.py by introducing strict type definitions (ManifestParamBase, SpecValue, etc.) and refactoring helper functions to use them. Two critical issues were identified in the review: first, using a generic alias (list[ManifestParamBase]) as a default_factory in ManifestStack will raise a runtime TypeError; second, the new strict type checking in _object_to_spec will raise a TypeError for previously supported types like tuple and custom mappings, which should be explicitly handled.

Comment thread src/firebase_functions/private/manifest.py Outdated
Comment thread src/firebase_functions/private/manifest.py
@IzaakGough IzaakGough requested a review from a team June 3, 2026 11:28
@IzaakGough IzaakGough marked this pull request as ready for review June 3, 2026 11:28
@cabljac cabljac self-requested a review June 8, 2026 09:35
Comment thread src/firebase_functions/private/manifest.py Outdated
@cabljac

cabljac commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

@IzaakGough nice cleanup, the typing is much tighter. A few things to address:

We can dismiss both gemini flags:

  • The list[T] default_factory one doesn't reproduce, list[str]() returns [] fine (generic aliases are callable), and you've already moved everything to default_factory=list anyway.
  • The tuple/Mapping concern is stale against your current code: _object_to_spec now handles _Mapping (custom mappings) and _Sequence excluding str/bytes (tuples), so they're covered.

The new else: raise TypeError (manifest.py:308) is the one real behavior change here, worth understanding before we merge. Old code was else: return data, so unsupported values used to pass straight through to yaml.dump (serving.py:117) and fail late. Failing fast is the better call, but whenever we tighten a fallthrough into a raise, we need to confirm it can't fire for real inputs. From what I can see it looks safe: the options layer seems to convert the tricky values into plain strings before they ever reach the manifest (e.g. timezone and enum options), so the new raise and your _ZoneInfo branch would only ever be hit by something unexpected, not by normal usage. Can you double-check that's actually the case across all the option/trigger types and confirm? I might be missing a path. Either way, good instinct to add the guard, just always pair that kind of change with a test that proves the practical path.

Which is the main ask: the diff is untested. Headline coverage is 91% but every line you changed behavior on is uncovered. Please add tests like these, a unit test documenting the guard plus a real-path test driving the actual API so we know it doesn't bite:

class TestObjectToSpec:
    def test_unsupported_type_raises(self):
        with pytest.raises(TypeError, match="Unsupported manifest spec value"):
            _manifest._object_to_spec({"value"})

    def test_zoneinfo_converts_to_key(self):
        assert _manifest._object_to_spec(ZoneInfo("America/New_York")) == "America/New_York"


class TestStrictSpecInPractice:
    def test_schedule_zoneinfo_timezone_builds_without_raising(self):
        import firebase_functions.options as _options
        options = _options.ScheduleOptions(
            schedule="every 5 minutes",
            timezone=_options.Timezone("America/New_York"),
        )
        endpoint = options._endpoint(func_name="test")
        spec = _manifest._dataclass_to_spec(endpoint)
        assert spec["scheduleTrigger"]["timeZone"] == "America/New_York"

@cabljac cabljac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice direction, see comments

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