Fix crashes on missing required fields; implement spec-defined PATCH/collection-GET; RFC 2046/2557 MIME fixes - #42
Conversation
POST and PUT on /mbs-user-services construct an MBSUserService (or call UserService::update(), which does the same) from the request body with no exception boundary around ModelException -- the C++ exception thrown by the generated model when a required field (extServiceIds, servType, servClass, servAnnModes, servNameDescs) is missing. Uncaught, it terminates the whole process, taking down every other active session with it, instead of returning the 400 the generated OpenAPI schema actually specifies for this case. Catches ModelException at both call sites and responds via the same ProblemDetails-shaped NfServer::sendError() path already used elsewhere in this file for validation failures.
GET on the bare /mbs-user-services, /sessions and /status-subscriptions paths (no ID) returned 400 "Invalid resource" unconditionally, even though TS 29.580 defines a real 200 (JSON array) response for all three -- confirmed against the actual generated OpenAPI spec, not just the generated-but-unwired API scaffolding. Each now returns the full current collection: MBS User Services and Ingest Sessions are iterated across each MBS User Service's own session map (there's no single flat map for sessions), while status subscriptions already had one and are read directly from it.
Both TS 29.580 MBSUserService and MBSUserDataIngSession declare a PATCH operation using RFC 7396 JSON Merge Patch (application/merge-patch+json), matching what rt-mbs-application-provider's client library already assumed -- but MBSF returned 400 "Invalid method" for the former and hard-coded a 404 "not allowed" for the latter. MBSUserService gets a UserService::modify() applying each present MBSUserServicePatch field onto the current MBSUserService, mirroring the working pattern already used for MBSUserDataIngStatSubsc's PATCH. MBSUserDataIngSession's mbsDisSessInfos entries need add/update/remove reconciliation against current state (preserving each session's mbsDistSessionId, flagging MBSTF updates, driving timers) that processUserDataIngSessionUpdate() already implements correctly for PUT -- rather than reimplementing that, the patch's present fields are overlaid onto the session's current full representation and handed to that same function. This also picks up the identical ModelException-crashes-the-process bug PUT already had, fixed here alongside it since both live in the same code path. One known limitation: a null value for a specific mbsDisSessInfos key (RFC 7396's per-key removal signal) isn't supported yet -- the generated MBSUserDataIngSessionPatch model used for up-front validation constructs a full MBSDistributionSessionInfo from every map entry unconditionally and has no representation for "this key was null", so it throws instead of validating. Removing a single distribution session without resending the others still requires PUT for now.
RFC 2557 SS4.1 defines Content-Location as a bare URI-reference, with no quoted-string form -- writing "Content-Location: \"announcement.json\"" makes the quote characters part of the value itself, which a strictly compliant parser would take literally, deriving an invalid reference rather than the intended relative filename.
random_string()'s charset included '@', '\', '!' and ';', none of which are members of RFC 2046 SS5.1.1's bcharsnospace grammar (the only characters a boundary value may contain, quoted-string wrapper or not). '\' was the more acute problem in practice -- written unescaped into the quoted-string Content-Type header parameter, it let a strictly RFC 2045-compliant quoted-string parser derive a different (unescaped) boundary value than the literal delimiter text actually used in the body, desyncing the two and making the whole entity unparseable -- but all four characters were illegal regardless of escaping.
@jordijoangimenez: Does the returned full collections has the resource id for each of the relevant resources returned? Please see the standards issue #191 |
Thanks for this implementation. Looking at #5 and #6, we decided not to implement PATCH operation. #6 specifically mentions that. I think the OpenAPI template did not support PATCH operation at that time. Hence would consider this to be an enhancement and not a bug. |
jordijoangimenez
added
enhancement
|
@devbbc, I think for GET we should wait for the standards fix. |
|
@jordijoangimenez wrote:
If you go on to read Section 4.4 in RFC 2557, which indicates how to "fold" a URI that contains other characters or that is too long for an RFC 822 header, it does allow the URI to be quoted to indicate that folding has occurred and therefore the value should be unfolded before being used. |
davidjwbbc
left a comment
davidjwbbc
left a comment
There was a problem hiding this comment.
Looking at this we have:
- Fixes 1 & 5 which look good (although I'd like to add in the extra permitted characters to fix 5).
- Fix 2 we deliberately didn't implement as there is current a pending Standards issue (5G-MAG/Standards#191) which addresses the changes needed to the structures used.
- Fix 3 we deliberately didn't implement as there is current a pending Standards issue (5G-MAG/Standards#182) which addresses the changes needed to the structures used.
- For fix 4, I believe that what is currently implemented is almost correct and that the quotes should be used as the names may contain characters that need quoting (e.g. spaces, colons, etc.), but the string needs escaping for \ and " for this to be done correctly.
I suggest we separate these 5 fixes out into separate issues, then close this PR as un-merged and create a new PR to address fixes 1 & 5. We can leave the issues for fixes 2 & 3 as backlog items to be considered once a decision has been made about each respective Standards ticket. Fix 4 can be reworked to escape the quoted strings properly and raised as a separate PR.
|
Replaced by PR #48 |
github-project-automation
Bot
moved this from Ready for review
to Done
in 5MBS: User Services initial release
5 participants
Summary
Five related fixes found and verified while building an MBS-4-MC-consuming client/UI against a live MBSF, cross-checked against the actual TS 29.580/26.517 OpenAPI spec (not just the generated-but-unwired API scaffolding) and the relevant IETF RFCs:
/mbs-user-servicesconstruct anMBSUserService(or callUserService::update(), which does the same) from the request body with no exception boundary aroundModelException-- the C++ exception the generated model throws when a required field (extServiceIds,servType,servClass,servAnnModes,servNameDescs) is missing. Uncaught, it terminates the whole process, taking down every other active session with it, instead of returning the 400 the generated OpenAPI schema actually specifies. CatchesModelExceptionat both call sites and responds via the same ProblemDetails-shapedNfServer::sendError()path already used elsewhere in this file./mbs-user-services,/sessionsand/status-subscriptionspaths (no ID) returned 400 "Invalid resource" unconditionally, even though TS 29.580 defines a real 200 (JSON array) response for all three. Each now returns the full current collection.MBSUserServiceandMBSUserDataIngSessiondeclare a PATCH operation using RFC 7396 JSON Merge Patch (application/merge-patch+json) -- but this returned 400 "Invalid method" for the former and a hard-coded 404 "not allowed" for the latter.MBSUserServicegets amodify()applying each present patch field, mirroring the working pattern already used forMBSUserDataIngStatSubsc's PATCH.MBSUserDataIngSession'smbsDisSessInfosreconciliation (preservingmbsDistSessionId, flagging MBSTF updates, driving timers) is reused fromprocessUserDataIngSessionUpdate()rather than reimplemented, by overlaying the patch's present fields onto the session's current full representation. Also fixes the identicalModelException-crashes-the-process bug on PUT, found in the same code path.mbsDisSessInfoskey (RFC 7396's per-key removal signal) isn't supported yet -- the generatedMBSUserDataIngSessionPatchmodel used for up-front validation constructs a fullMBSDistributionSessionInfofrom every map entry unconditionally and has no representation for "this key was null", so it throws instead of validating. Removing a single distribution session without resending the others still requires PUT.Content-Location: "announcement.json"makes the quote characters part of the value itself.random_string()'s charset included@,\,!and;, none of which are members of RFC 2046 §5.1.1'sbcharsnospacegrammar (the only characters a boundary value may contain, quoted or not).\was the more acute problem in practice -- written unescaped into the quoted-stringContent-Typeheader parameter, it let a strictly RFC 2045-compliant quoted-string parser derive a different (unescaped) boundary value than the literal delimiter text actually used in the body, desyncing the two and making the whole entity unparseable -- but all four characters were illegal regardless of escaping.Test plan
mainServLangchange verified end-to-end through a client), and the MIME fixes observed producing clean, RFC-compliant boundaries/Content-Location with zero parse failures across multiple carousel cycles (previously failing within ~1 minute)