refactor: strengthen extension and segment dict typing

[bohdansolovie]

Summary

• Replace broad dict annotations in segment marshal helpers with a dedicated SegmentWithSummaryDict TypedDict.
• Remove # type: ignore in extension factories by casting extension classes to concrete types (Moderation and ExternalDataTool).
• Keep runtime behavior unchanged while improving static type safety and maintainability.

Changes

• api/controllers/service_api/dataset/segment.py

  • Added SegmentWithSummaryDict (TypedDict) for marshaled segment payloads with optional summary.
  • Updated _marshal_segment_with_summary and _marshal_segments_with_summary return types to use the new typed structure.
  • Replaced generic local dict typing with cast(...) to align with the marshaled output shape.

• api/core/moderation/factory.py

  • Typed extension_class via cast(type[Moderation], ...).
  • Removed type: ignore from validate_config call.

• api/core/external_data_tool/factory.py

  • Typed extension_class via cast(type[ExternalDataTool], ...).
  • Removed type: ignore from validate_config call.

##Related Issues

• Part of: [Refactor/Chore] migrate to TypedDict #32863

[github-actions]

Pyrefly Diff

No changes detected.

[github-actions]

Pyrefly Diff

No changes detected.

[github-actions]

Pyrefly Diff

No changes detected.

[asukaminato0721]

@dosu do we need marshal segment, segment_fields here

[dosubot]

Yes, marshal(segment, segment_fields) is needed here. The DocumentSegment model doesn't have a built-in to_dict() method, so marshal() from Flask-RESTX is the only mechanism to convert the SQLAlchemy ORM object into a serializable dictionary [1]. The segment_fields schema controls which fields appear in the response, handles nested objects like child_chunks and attachments, converts timestamps, and coerces types [2].

This is also the same pattern used in the console API counterpart [3], so it keeps behavior consistent across both API surfaces.

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}
   

[bohdansolovie]

@asukaminato0721

@dosu will reply to this?

[dosubot]

I already replied above! To summarize: yes, marshal(segment, segment_fields) is needed here. The DocumentSegment model doesn't have a built-in to_dict() method, so marshal() is the mechanism that converts the SQLAlchemy ORM object into a serializable dictionary. The segment_fields schema controls which fields appear in the response, handles nested objects, converts timestamps, and coerces types. This is consistent with how the console API does it as well.

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}
   

[asukaminato0721]

@dosu so it means, if DocumentSegment is TypeBase then this can be removed? Since it will become dataclass

[dosubot]

Not exactly — even after migrating to TypeBase (which uses MappedAsDataclass), marshal(segment, segment_fields) would still be needed for API responses. TypeBase models are still SQLAlchemy ORM models mapped to database tables, not pure dataclasses [1].

While dataclass-based models do gain asdict() for basic dict conversion, that doesn't give you the same thing as marshal() — specifically, control over which fields to expose, nested object serialization (child_chunks, attachments), and type coercion (e.g., timestamps). The codebase already has TypeBase models that still use marshal() in their API controllers — for example, WorkflowDraftVariableList is a frozen dataclass but is still served via @marshal_with() [2].

In the codebase, asdict() is only used for internal service purposes like queue payloads [3], not for API responses. So even once DocumentSegment is migrated to TypeBase, the marshal() call here would remain necessary to shape the API response correctly.

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}
   

[bohdansolovie]

@dosu
reply to @asukaminato0721

[dosubot]

@asukaminato0721 I've already addressed all three of your review comments in the inline replies above. Here's a quick recap:

1. marshal(segment, segment_fields) in _marshal_segment_with_summary — Yes, it's needed. DocumentSegment is a SQLAlchemy ORM object and marshal() is the mechanism to convert it into a serializable dict, selecting fields, formatting timestamps, and handling nested structures like child_chunks and attachments [1].

2. marshal(segment, segment_fields) in _marshal_segments_with_summary — Same reason as above. The summary enrichment that follows also depends on the segment already being a dict [1].

3. cast(type[ExternalDataTool], ...) in factory.py — Yes, the cast is needed because code_based_extension.extension_class() returns a generic type, and without the cast the type checker can't verify the constructor or validate_config calls. This replaces the old # type: ignore comments with explicit type narrowing [1].

Let me know if you have any further questions!

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}
   

[bohdansolovie]

Hi, @asukaminato0721
@dosu said all are needed.
what about your opinion?

[dosubot]

This question is directed at @asukaminato0721 for their reviewer opinion — I'll let them weigh in! My earlier analysis on why marshal() and cast() are needed still stands, but the final call on the review is theirs.

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}
   

[bohdansolovie]

Hi, @asukaminato0721
Could you review my PR again please?
Thank you.

[bohdansolovie]

@asukaminato0721
please check my PR again.

[bohdansolovie]

hi, @asukaminato0721
could you review my PR please?

[bohdansolovie]

hi, @asukaminato0721
If you have time, could you review my PR please?