dependency-function

[peterschutt]

• defines Dependency in params.py
• Dependency inserts is_dependency=True into field.extra
• excludes explicit dependencies from parameter documentation.
• Raises ImproperlyConfiguredException when an explicit dependency has no default and is not provided to a route handler.

[peterschutt]

Closes #128
Related #136

[peterschutt]

• Implement Dependency function - consider the params that we expose from Field.
• Implement skip declared dependencies for parameter documentation if the dependency not provided.
• Build logic for checking that all declared dependencies are either provided or have a default value.
• Docs for Dependency, include example.

Test cases:

• In/out tests for Dependency function that tests the stuff we rely on in extra is there.
• declared optional type and undefined default, e.g., dep: int | None = Dependency()
• non-optional type and defined default, e.g., dep: int = Dependency(default=30)
• error case, e.g., dep: int = Dependency() and dep not provided
• test that parameters for dependencies reach the docs if the dependency is provided.
• test that the default values reach the dependency if not provided.
• some tests of validation Field parameters.
• test that any validation failures of dependency fields are 500s not 400s - if a dependency fails validation it isn't the client's fault.

[Goldziher]

Please check sonar - also coverage is below 100%.

[peterschutt]

Yeh this isn't ready yet.

I pushed up the impl so that I could open a PR to move my checklist from the issue into here.

Working on the tests now.

[Goldziher]

Yeh this isn't ready yet.

I pushed up the impl so that I could open a PR to move my checklist from the issue into here.

Working on the tests now.

Sorry. Will hold back next time. To avoid confusion, best to use a draft PR

[peterschutt]

It is draft ;) although I marked it draft a few seconds after I created it. So Sonar should not run on drafts?

[Goldziher]

It is draft ;) although I marked it draft a few seconds after I created it. So Sonar should not run on drafts?

It does. Anyhow, sorry

[peterschutt]

No problems at all.

Does it make too much noise for you if I work directly in the repo? I can be doing a lot of this in my fork if that's better?

[Goldziher]

Not at all.

I get a lot of notifications usually - work stuff.

[peterschutt]

I've cooled on allowing validation arguments to Dependency for now.

For one thing it isn't clear what error should be returned to the client. Probably should be a 500 error by default if an explicit dependency fails validation, however that might need to be configurable, maybe Dependency(lt=13, invalid_status=400).

It would also be possible to have both a client parameter validation failure and a dependency validation failure on the same signature model for a given request. In that case we'd probably have to return the higher status code of 400 or the status for the dependency validation failure.

By excluding the validation kwargs from Dependency altogether for now it means we can add them in later on when someone requests them with a concrete use case and not break anything. It would be much harder to take them away or even worse to have to maintain support with edge cases everywhere forever.

There is also the issue of identifying that the field that raised the ValidationError is a Dependency field in SignatureModel.parse_values_from_connection_kwargs(). I'm sure it is possible, but case where the dependency is buried a few levels below the surface of the dependency tree of the handler might be be tricky.

[peterschutt]

@Goldziher I still have to do the docs, but code is ready for review.

[Goldziher]

@Goldziher I still have to do the docs, but code is ready for review.

Ok, will review shortly. Please look in sonar and increase coverage to 100%.

[Goldziher]

this value can be placed in a variable and then used in both calls.

[Goldziher]

clever.

[Goldziher]

Looks good 🚀

[peterschutt]

Please look in sonar and increase coverage to 100%.

The one line that has no coverage in sonar is the else block for if sys.version_info >= (3, 10):. Sonar must be running on 3.10. Is it coverage.py under the hood (I.e, # pragma: no cover to exclude the line from coverage)?

[Goldziher]

Yup

[peterschutt]

I ran into a chicken and egg scenario where I need the name of all provided dependencies to check for the error condition prior to the signature model being constructed (because I need them to check the signature model themselves). That prevents me from using route_handler.resolve_dependencies() to get the names of the dependencies. So I added to BaseRouteHandler:

    @property
    def dependency_name_set(self) -> Set[str]:
        """
        The set of all dependency names provided in the handler's ownership layers.

        Intended as a fast to compute set of the names of dependencies provided to the handler, and
        available at the time that the handler's signature model is generated. Full resolution of
        dependencies requires that the signature model is already generated and is performed in
        ``BaseRouteHandler.resolve_dependencies()``.
        """
        if self.resolved_dependency_name_set is BaseRouteHandler.empty:
            self.resolved_dependency_name_set = {
                k
                for k in itertools.chain.from_iterable(
                    (layer.dependencies or {}).keys() for layer in self.ownership_layers()
                )
            }
        return cast(Set[str], self.resolved_dependency_name_set)

Also added a section to the bottom of the usage docs in 6-dependency-injection.md.

LMK any changes you'd like and I'll try to close this one out so I can clean up the other couple of PRs that are there.

Would you prefer a squash and merge for a PR like this one?

[Goldziher]

Great stuff. Merge when ready .

[Goldziher]

Please split this into variable rather than nest it

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

100.0% Coverage
0.0% Duplication