Skip to content

ref(alerts): Refactor DetailedWorkflowEngineDetectorSerializer to remove inheritance#110706

Merged
kcons merged 1 commit intomasterfrom
kcons/fixsubcl
Mar 14, 2026
Merged

ref(alerts): Refactor DetailedWorkflowEngineDetectorSerializer to remove inheritance#110706
kcons merged 1 commit intomasterfrom
kcons/fixsubcl

Conversation

@kcons
Copy link
Member

@kcons kcons commented Mar 13, 2026

Implemenetation inheritance is always a little iffy, and this one (at least according to mypy) was a LSP violation.
Better to just fix it than to try to cast it away.

@kcons kcons requested a review from a team as a code owner March 13, 2026 23:48
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 13, 2026
Comment on lines +488 to +489
base_data = self.base_serializer.serialize(obj, attrs, user, **kwargs)
data: DetailedAlertRuleSerializerResponse = {
Copy link
Contributor

@saponifi3d saponifi3d Mar 13, 2026

Choose a reason for hiding this comment

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

really like how this reads in code!

any thoughts on ways we might be able to support other folks in knowing the best path forward for these? (iirc the hope for using inheritance was to reduce the number of touch points required for consumers of the platform to modify).

Should the base class follow a registry pattern? should we create flat helper methods? should just create an abc that says "add the base_validator" property like this? 🤔 lots of ways to approach it; i think the inheritance pattern was the established one, but i'm not a big fan of "lasagna" code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any great ideas, but I think:

  • Treat cast and cycle-breaking import as structural bugs. Not all solvable, but all worth solving.
  • Treat implementation inheritance for code-sharing with extreme suspicion; can be fine, but in the claude era we don't need to feel too bad about well-specified boilerplate translation layers.

But this particular case was my fault, so I'm happy to be able to fix it.

For serializers and validators in general, I think the support for hierarchical composition for reuse is actually really strong. The case for inheritance based code sharing (particularly with validators) is much weaker. Absent a graphql sort of world, we may want to be leaning more toward structures that allow extending to be a explicit composition (eg one key for X fields, one key for additional data, rather than merging them into one object, even though that's possible). I haven't thought enough about it to have a worthwhile opinion, though.

That said, I'm currently very in favor of avoiding ever using partial=True for Validators. Having a mode where required stops meaning anything is convenient in some cases, but makes the code really hard to make reliable.

@kcons kcons merged commit 7a70158 into master Mar 14, 2026
58 checks passed
@kcons kcons deleted the kcons/fixsubcl branch March 14, 2026 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants