Refactor/mcp document creation - #9018
Merged
Merged
Conversation
The MCP tools and the AI media storage service created course documents by fabricating a Request and invoking CreateDocumentFileAction as a callable with its ten resolved dependencies. Nothing about that was a request: no routing, no firewall, no CidReqListener, no security expression, no validation, no serialization, no state processor. Only the body travelled, so "it goes through the API" bought none of the API's guarantees -- both callers authorize on their own, and have to. What it did buy was a parameter bag of thirteen untyped strings and a hard coupling to the controller's signature. That coupling failed twice this month, in silence: when the action gained a CidReqHelper parameter both callers kept passing AiDisclosureHelper into its slot, and once that was fixed the documents still came out with no course link, because CidReqHelper reads the request stack rather than the request it is handed. So the logic moves to CourseDocumentCreator, with two entry points over one implementation: create(CourseDocumentInput, Course) for callers that are not a request, and createFromRequest(Request) for the action, which passes its own request through untouched -- the HTTP path is unchanged by construction. CreateDocumentFileAction is now twenty-six lines and delegates. The cloud-link host allowlist moves with the logic it guards. The callers lose the dependencies they only ever forwarded: the MCP tool goes from nine constructor arguments to six, the media service from nine to two. One seam is deliberately left: handleCreateFileRequest() still reads everything off a Request, so create() builds one -- but once, inside the service, behind a typed boundary, instead of at every call site. Giving that method its own input touches its five callers (CreatePersonalFileAction, CreateStudentPublicationFileAction, CreateStudentPublicationCorrectionFileAction, FileManagerController and this one), which is the next step, not this one. The docblock says so. testCreatedDocumentsAreLinkedToTheCourse pins what started this: a document an MCP call creates carries exactly one resource link, to the course, with no session and no group.
handleCreateFileRequest() took a Request and read thirteen values off it, so the
only way to create a resource that carries a file was to have a request -- or to
fake one, which is what CourseDocumentCreator::create() was still doing after the
last commit gave its callers a typed boundary.
It is now three things:
- handleCreateFile() holds the ~190 lines that do the work and reads nothing off a
request;
- resourceFileInputFromRequest() maps a request into the ResourceFileInput it
takes -- the JSON body, the POST fields, the bracket-less single link, the
forced link list, the uploaded file;
- handleCreateFileRequest() is a shim with the old signature, so the four HTTP
callers -- CreatePersonalFileAction, the two student-publication actions and
FileManagerController -- are untouched.
Two fields keep a distinction the old code expressed with
$request->request->has(): a null contentFile means none was sent, while '' is an
empty HTML editor save and a real create; a null language leaves the resource
alone, while '' means "the course's". ResourceFileInputFromRequestTest pins those
and every other quirk of the reading that was replaced, because that mapper is
the only thing standing between the HTTP callers and a behaviour change.
The typing pays for itself in the language fallback: it used to find the course
through $request->query->getInt('cid'), and now takes the course it was given, or
the one the link list is already bound to -- the authorized course rather than a
raw parameter. That was the last reader of the query string in this path, so
create() builds no request at all any more.
CourseDocumentCreator and CourseDocumentInput move to Mcp/ as asked. The namespace
is not a reachability claim -- POST /api/documents and GeneratedMediaStorageService
go through the creator too, and its docblock says so. ResourceFileInput stays in
Dto/: it serves personal files, student publications and links as much as
documents.
CoreBundle 783 tests green, CourseBundle 114.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9018 +/- ##
============================================
+ Coverage 13.27% 13.30% +0.02%
- Complexity 57499 57528 +29
============================================
Files 2323 2326 +3
Lines 218458 218549 +91
============================================
+ Hits 29002 29073 +71
- Misses 189456 189476 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.