Fix subdomain inheritance for nested blueprints. #4856
Conversation
| if bp_subdomain is None: | ||
| bp_subdomain = blueprint.subdomain | ||
|
|
||
| if state.subdomain is not None and bp_subdomain is None: |
There was a problem hiding this comment.
I think we should also consider what to do if state.subdomain is not None and bp_subdomain is not None. I think we should combine as state.bp (much like with the prefix). Would you expect it to work this way as well?
There was a problem hiding this comment.
Yes good point.
I thought that if both parent and child have subdomain set, it would be weird to combine. It makes sense for paths but subdomains in the form foo.bar.baz are rare.
For example if parent is bar and child is foo, we would get foo.bar.
But I can get this by supplying foo.bar in the child blueprint subdomain. So I went with the child overrides parent subdomain way that way the user can use a combined subdomain as needed.
Not sure this is a good idea though because some use cases might need them to be combined by default.
Another possible solution would be to introduce an extra parameter called something like "combine_subdomains". This will also allow different child blueprint to have different subdomain strategies. For example, foo can choose to be combined but baz can choose not to.
What do you think?
There was a problem hiding this comment.
Given it might confuse it is probably safest to raise a ValueError/RuntimeError then if both are set. (This would be raised during initialization and so isn't likely to cause any issues for existing apps)
There was a problem hiding this comment.
It could be an issue if the user wants to combine the subdomain anyway.
For example, parent blueprint's subdomain is bar then user wants child blueprint X to be available at "foo.bar" so he sets the subdomain. Then another blueprint Y needs to be at "baz.bar" so he sets the subdomain like that. But parent needs to be available at bar subdomain no matter what.
Raising ValueError would break this use case since both parent and child subdomains are set.
There was a problem hiding this comment.
I've had quite a bit of time to think about this, and to talk to others about their expectations. I think we should combine the subdomains (much like we do for the paths). I think this is the clearest expectation for a nested blueprint i.e. that the routes defined within it nest both by path and subdomain. Could you update this PR to do so and then I'll merge.
|
continued in #4935 |
When child blueprints are mounted onto parents, they don't inherit the subdomain set by the parent.
This PR ensures that routes in child blueprint will be available at the same subdomain set by the parent.
However, children can also specify their own subdomain if necessary.
Checklist:
CHANGES.rstsummarizing the change and linking to the issue... versionchanged::entries in any relevant code docs.pre-commithooks and fix any issues.pytestandtox, no tests failed.