feat(nfConfigApi): add guaranteed bit rate to PccQos - #176
Conversation
Hi @midwell, thank you for your contribution. I am going to review this PR (late) next week and provide my feedback. Again, thanks! |
There was a problem hiding this comment.
Pull request overview
Adds guaranteed bit rate support to the NFConfig API’s PccQos model so policies can express a committed (floor) rate alongside the existing maximum (ceiling) rate.
Changes:
- Extend
PccQosOpenAPI schema with optionalgbrUl/gbrDlfields. - Extend generated Go model
nfConfigApi.PccQoswith optionalGbrUl/GbrDlfields, including (un)marshalling and accessors. - Add Go tests verifying round-trip serialization, omission when unset, and correct decoding (not leaking into
AdditionalProperties).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| nfConfigApi/webconsole-api.yaml | Adds gbrUl/gbrDl properties to the PccQos schema. |
| nfConfigApi/model_pcc_qos.go | Adds GbrUl/GbrDl fields plus getters/setters and JSON map/(un)marshal handling. |
| nfConfigApi/model_pcc_qos_gbr_test.go | Adds tests covering the new guaranteed bit rate fields’ JSON behavior. |
Files not reviewed (1)
- nfConfigApi/model_pcc_qos.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // An unset guaranteed rate must not land in AdditionalProperties, which is where unknown keys | ||
| // go — that would make it look like a field the model does not know about. |
@midwell, I just regenerated the code using openapi-generator and the output was the same you have in your changes/PR, of course, without the |
PccQos carries maxBrUl and maxBrDl but no guaranteed rates, so a policy served to the PCF can express a ceiling and not a floor. WebConsole already accepts GBRUL and GBRDL on its flow rules and stores them; they have nowhere to go from here, and are dropped on the way out. gbrUl and gbrDl are optional strings, treated exactly as the maximum rates beside them: no pattern, since the existing bit rate fields carry none either. The model and the spec are updated together. There is no committed generator configuration for this package, so the spec is kept in step by hand and a future regeneration keeps the fields rather than dropping them. Tests cover the round trip, omission when unset, and that a supplied value lands in the field rather than in AdditionalProperties — the last verified to fail without the delete() calls in UnmarshalJSON, which is the part easiest to leave out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyNr6vp6JaxxzPyVcuHXTf Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
7931666 to
2e793fc
Compare
|
Thanks for regenerating — good to know the generator reproduces the model exactly; that was the part I was least sure of. On the test file: I put it here because it covers code that lives here, so if a future regeneration ever drops I've also fixed the misleading comment Copilot flagged: that test decodes a One other thing, separate from the review. I've added a second commit dropping The CI run is waiting on maintainer approval, so the checks haven't reported yet. |
This is expected because this is your first contribution in this repo. After this PR is merged, your future contributions in this repo will automatically run the CI pipeline. |
My concern about the test file is that if somebody regenerates the models in the future, the easiest way to do it might be by deleting the entire directory |
This change is ok to be part of your PR |
2e793fc to
f753f14
Compare
The webconsole and pcf changes that populate gbrUl/gbrDl cannot be opened until these fields are in a released openapi, so drop -dev to have the pipeline tag 2.2.1 on merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The reviewer's objection was that this is the only model_xxx_test.go in the repo: openapi-generator does not produce them, so it reads as unexplained hand-written code inside a generated tree, and a regeneration that replaces the directory takes it with no sign it was deliberate. His alternative of an api_default_test.go does not hold either. All 311 api_xxx_test.go files in this repo carry the generator's DO NOT EDIT header - that name is the generator's own output slot, one per api file in every package, and nfConfigApi has simply never had one. Hand-written assertions there would be overwritten silently rather than merely deleted. So the assertions go where he suggested, the consumer: webconsole's backend/nfconfig/pcc_qos_gbr_test.go already exercises the same fields through buildPccQos, including the one-directional case, and arrives with the change that populates them. This PR is left as the generated shape and the release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
f753f14 to
0d25c35
Compare
|
Good point. I have taken your other suggestion and dropped the test from this PR. It costs nothing, because the consumer already covers it: webconsole's backend/nfconfig/pcc_qos_gbr_test.go exercises the same fields through buildPccQos — carried, omitted when unset, and the one-directional case — and configapi/slice_helpers_gbr_test.go covers the unit normalisation. Those arrive with the webconsole change that populates the fields, once 2.2.1 is released. This PR is now the generated shape and the release commit only: model_pcc_qos.go, webconsole-api.yaml, VERSION. |
What
Adds
gbrUlandgbrDlto thePccQosschema innfConfigApi/webconsole-api.yamland thecorresponding optional
GbrUl/GbrDlfields tonfConfigApi.PccQos, beside the existingMaxBrUl/MaxBrDl.Why
PccQoscan express a maximum bit rate per flow but not a guaranteed one, so a policy servedthrough this API can only ever describe a ceiling. A GBR 5QI needs both: TS 23.501 table 5.7.4-1
defines GFBR and MFBR as separate QoS parameters for GBR and delay-critical GBR resource types,
and the SMF needs the guaranteed value to populate the GBR IE toward the UPF and the QoS flow
description toward the UE.
Concretely this is what lets WebConsole publish a committed rate alongside the peak rate, so a
deployment can offer a service with a floor rather than only a cap. Both fields are optional, so a
Non-GBR flow carries neither and serialises exactly as it does today.
Note on regeneration
The models carry
DO NOT EDIT, so I want to be transparent that this field was hand-written, andabout how I checked it.
The repository commits no generator configuration, script or make target, and
nfConfigApiis theonly package without a
README.mdrecording its generator version. A stock run of the generatordiffers from the committed files by 40 to 91 lines in every one of the 17 models in this package:
disallowAdditionalPropertiesIfNotPresent=falseis needed just to emit theAdditionalPropertiessupport the committed code has, and after that around 70 lines per file remain that look like
post-processing this repository applies —
openapi.IsNilandopenapi.MappedNullablefrom theparent module in place of the package-local copies the generator emits,
yaml:tags beside everyjson:tag,maps.Copyin place of the generated loop, and the SPDX headers.So regenerating would produce a large diff unrelated to this change. Instead I used the generator
as an oracle: generated the package from the spec before and after this change, diffed generated
against generated to isolate exactly what the generator emits for two optional string fields,
applied the transformations above, and compared that to the diff in this PR. They are identical,
48 lines each. Comparing deltas rather than whole files makes the check independent of generator
version, which mattered because the version available to me was 7.24.0 while the module records
7.20.0.
If maintainers would rather this came from a real regeneration, I am happy to redo it — and if the
invocation and post-processing were captured in a script, I would be glad to contribute that
separately, since it is currently not reproducible from the repository alone.
Tests
Three tests in
nfConfigApi/model_pcc_qos_gbr_test.go:gbrUlin an incoming body decodes into the field rather than landing inAdditionalProperties, which is what thedelete()lines controlThe last one is mutation-verified: removing the
delete(additionalProperties, "gbrUl")line makesit fail.
golangci-lint runreports 0 issues for the module andgo test ./... -race -coverpasses.🤖 Generated with Claude Code
https://claude.ai/code/session_01GyNr6vp6JaxxzPyVcuHXTf