Add srvb srvd crud#178
Merged
Merged
Conversation
Service Definitions (SRVD/SRV) carry their CDS source body at
.../source/main with Content-Type text/plain - exactly the same shape
as DDLS, programs, data elements and similar objects. The current
OBJTYPE declared empty typeuris and no editor_factory, so neither
`obj.text` nor `obj.open_editor()` worked end-to-end.
Set typeuris={'text/plain': 'source/main'} and editor_factory to
ADTObjectSourceEditor.plain_text. The standard source-editor code path
takes care of GET/PUT against the source URI and the lock handshake.
Add unit tests proving the round-trip - read returns the body verbatim,
write PUTs it to .../source/main with the correct headers and lockHandle.
A bare `ServiceBinding(connection, name)` POST is rejected by the ADT
back-end with PreconditionFailed ("Provide valid details for Service
Binding creation") - the server requires the binding shape (type,
version, category, implementation) AND a wired service entry to be
present in the create body. The legacy two-step "create-empty,
then-mutate" dance does not work; v1 must POST a fully-formed binding
in one request.
Extend ServiceBinding.__init__ to accept the missing pieces:
- package sets adtcore:packageRef
- typ srvb:type (ODATA, INA, SQL)
- version srvb:version (V2, V4, 1)
- service_name adds one <srvb:content><srvb:serviceDefinition.../>
- service_version defaults to '0001' (matches what the server fills in)
When typ/version/service_name are unset the constructor stays a no-op
beyond setting the package, which preserves backwards compatibility for
the existing `rap binding publish` callers (covered by the new
test_init_without_typ_version_keeps_binding_unset test).
The marshalled POST body matches the live-captured request shape
exactly - verified by test_init_creates_full_post_body which feeds the
serialised XML through the mock Connection and asserts the request
body byte-for-byte.
Add the top-level `srvd` command group for Service Definition (SRVD/SRV) CRUD. Mirrors sap/cli/datadefinition.py - a thin CommandGroupObjectMaster subclass that hands off to sap.adt.ServiceDefinition. The inherited create/read/write/activate/delete/whereused commands work without overrides because the previous commit enabled the text/plain source editor on the ADT class. User-facing surface: sapcli srvd create NAME DESCRIPTION PACKAGE [--corrnr CORRNR] sapcli srvd read NAME sapcli srvd write NAME [FILEPATH+|-] [-a] [--corrnr CORRNR] ... sapcli srvd activate NAME [NAME ...] sapcli srvd delete NAME [NAME ...] [--corrnr CORRNR] sapcli srvd whereused NAME Register the group in sap/cli/__init__.py next to the existing `rap` alias, and add doc/commands/srvd.md mirroring the ddl.md layout.
Add the top-level `srvb` command group for Service Binding (SRVB/SVB).
The binding has no text/plain source body so the inherited `write`
command from CommandGroupObjectMaster does not apply - override
define_write to return None and lock the v1 contract with an explicit
test (test_cli_srvb_has_no_write_command). v2 will replace this with a
JSON round-trip implementation.
`srvb create` requires --type, --version, --service flags. The
back-end rejects bindings without a wired service entry with
ExceptionPreconditionFailed ("Provide valid details for Service
Binding creation"), so the CLI never offers an "empty binding" mode.
--service-version defaults to '0001' which matches the value the
server fills in for fresh bindings.
`srvb read` prints a structural summary (name / description / package
/ type / version / published / services list) instead of source code,
because there is no source body to print.
User-facing surface:
sapcli srvb create NAME DESCRIPTION PACKAGE
--type {ODATA,INA,SQL} --version {V2,V4,1}
--service SRVD_NAME [--service-version VER]
[--corrnr CORRNR]
sapcli srvb read NAME
sapcli srvb activate NAME [NAME ...]
sapcli srvb delete NAME [NAME ...] [--corrnr CORRNR]
sapcli srvb whereused NAME
Register in sap/cli/__init__.py and add doc/commands/srvb.md.
Publish lands in the next commit.
Add `sapcli srvb publish BINDING_NAME [--service NAME] [--version VER]`.
The publish-job logic moves verbatim from sap.cli.rap.publish - same
single-service short-circuit, same multi-service filter handling, same
exit codes and error messages.
Implementation lives in two pieces:
- `sap.cli.srvb.publish_binding(connection, args)` is a module-level
helper containing the moved logic. Keeping it as a free function
rather than a CommandGroup method lets the rap deprecation shim
(next commit) call it without instantiating a srvb CommandGroup.
- A thin `@CommandGroup.command('publish')` wrapper hooks the helper
into the srvb subparser. The class-level decorator runs at module
load and adds 'publish' to the same CommandsList that
CommandGroupObjectMaster.define() populates with the CRUD commands.
The existing ServiceBinding.publish() method on sap/adt builds the
correct per-binding URL (`businessservices/{type}{version}/publishjobs`)
already - no ADT-layer change needed.
The new top-level `srvd` and `srvb` command groups own SRVD/SRVB
CRUD now. `sapcli rap definition activate` and `sapcli rap binding
publish` become deprecated aliases that:
1. write a single-line warning to stderr identifying the new command
and announcing removal in the next minor release, and
2. delegate to the new code path (no logic duplication).
`rap binding publish` calls into sap.cli.srvb.publish_binding directly.
`rap definition activate` instantiates sap.cli.srvd.CommandGroup() and
calls its inherited activate_objects, after defaulting
ignore_errors / warning_errors to False (the rap subparser does not
declare those flags - the legacy worker had no equivalent settings).
The existing happy-path tests are updated to expect the deprecation
warning prepended to stderr; two new tests
(test_rap_binding_publish_emits_deprecation_warning and
test_rap_definition_activate_emits_deprecation_warning) lock in the
warning-once contract.
doc/commands/businessservice.md is rewritten as a deprecation pointer
to the new srvd.md / srvb.md docs.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two new CLI command groups ( ChangesSRVD/SRVB CLI commands + rap deprecation shims
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/test_sap_adt_businessservice.py (1)
303-350: ⚡ Quick winAdd negative tests for invalid
ServiceBindinginit combinations.Current coverage validates happy paths only. Please add cases for
(typ without version),(version without typ), and(service_name without typ/version)so constructor input validation can’t regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_adt_businessservice.py` around lines 303 - 350, Add three new negative test methods to validate that ServiceBinding constructor properly rejects invalid parameter combinations. Create test_init_with_typ_without_version, test_init_with_version_without_typ, and test_init_with_service_name_without_typ_version methods that use assertRaises to verify that ServiceBinding raises an appropriate exception when initialized with these invalid combinations. This ensures the constructor's input validation prevents silent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sap/adt/businessservice.py`:
- Around line 117-136: The constructor currently accepts partial binding
configuration by using an OR condition when checking if typ and version are
provided, and allows service_name to be set without a corresponding binding
configuration, which can generate invalid XML payloads. Change the condition in
the inner_binding creation block from checking if typ is not None or version is
not None to ensuring both typ AND version are provided together, and add
validation to ensure that if service_name is provided, the required binding
configuration must also be present. This prevents building incomplete XML
structures and catches configuration errors locally before attempting server
requests.
In `@sap/cli/srvb.py`:
- Around line 109-117: The error messages in the SRVB publish validation contain
a typo where "Biding" is used instead of "Binding". Locate all error messages
that display user-facing text to the console (particularly those using
console.printerr) and correct the spelling from "Business Service Biding" to
"Business Service Binding". This includes the error message that checks if the
binding contains no services and the error message that appears when publishing
without service definition filters.
In `@test/unit/test_sap_cli_srvb.py`:
- Line 7: Remove the unused `call` import from the `unittest.mock` import
statement at the top of the file. The `call` object is imported in the line that
imports from `unittest.mock` but is never referenced anywhere in the test file,
so it should be removed from the import list while keeping `patch` and `Mock`.
---
Nitpick comments:
In `@test/unit/test_sap_adt_businessservice.py`:
- Around line 303-350: Add three new negative test methods to validate that
ServiceBinding constructor properly rejects invalid parameter combinations.
Create test_init_with_typ_without_version, test_init_with_version_without_typ,
and test_init_with_service_name_without_typ_version methods that use
assertRaises to verify that ServiceBinding raises an appropriate exception when
initialized with these invalid combinations. This ensures the constructor's
input validation prevents silent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b44732a-0446-4100-bc81-c263598bd37f
📒 Files selected for processing (13)
doc/commands/businessservice.mddoc/commands/srvb.mddoc/commands/srvd.mdsap/adt/businessservice.pysap/cli/__init__.pysap/cli/rap.pysap/cli/srvb.pysap/cli/srvd.pytest/unit/fixtures_adt_businessservice.pytest/unit/test_sap_adt_businessservice.pytest/unit/test_sap_cli_rap.pytest/unit/test_sap_cli_srvb.pytest/unit/test_sap_cli_srvd.py
Contributor
|
Tested PR #178 head ( ✅ What works
🔴 Bug 1 —
|
The back-end rejects POSTs that lack srvd:srvdSourceType:
ExceptionResourceCreationFailure: Service Definition type '' does
not exist
Caught the hard way against a live system - sapcli srvd create
was failing 100% of the time. The spec
(features/srvd_srvb_crud/srvd_srvb_crud.md, lines 142-143) flagged
this requirement explicitly when the feature was reviewed; the
implementation missed it.
Add a class-level default:
source_type = XmlNodeAttributeProperty('srvd:srvdSourceType', value='S')
'S' (= "Definition") is the only value observed in live captures.
The attribute is positional (XmlNodeAttributeProperty), so it
deserialises from GET responses too, which lets the existing
EXAMPLE_CONFIG_SRV fixture double-check the field round-trips.
Why the unit tests missed it: SERVICE_DEFINITION_ADT_POST_REQUEST_XML
was generated from the broken Marshal().serialize() output instead of
copied from the wire capture in srvd_srvb_crud.md - the fixture
matched the bug rather than the contract. This patch:
- updates SERVICE_DEFINITION_ADT_POST_REQUEST_XML to include the
required attribute (the test_servicedefinition_create_serializes_post_body
assertion already uses full assertEqual, so the new fixture catches
any future regression),
- adds test_servicedefinition_default_source_type_is_S to lock in the
in-memory default,
- extends test_binding_fetch (the GET round-trip) to assert
source_type='S' so the property stays wired through deserialisation.
…captures
Audit pass against the live e2e captures in
features/srvd_srvb_crud/captures/scaffold/. Two structural divergences
were caught between the fixtures I committed earlier and the actual
wire bytes the back-end exchanges:
(1) SERVICE_BINDING_ADT_GET_V4_XML lacked srvb:allowedAction on the
root, srvb:minorVersion / srvb:patchVersion / srvb:buildVersion on
<srvb:content>, and the <srvb:bindingTypeData><adtcore:content
encoding="base64"/></srvb:bindingTypeData> child. The marshaller
silently ignores attributes/elements it does not model, so the read
summary keeps working - but the fixture is supposed to mirror the
server, not the parser. Updated to match 06b_srvb_get_post_activate
verbatim (modulo object name).
(2) SERVICE_BINDING_ADT_POST_ODATA_V4_REQUEST_XML and the marshalled
body itself lacked <srvb:services srvb:name="...">. The captured
create request 05b_srvb_create_wired_request.xml shows it is
always sent and equals the parent binding's name. The previous
XmlContainer.define-only ServicesContainer factory cannot carry
container-level attributes, so introduce ServicesContainerWithName
- a direct subclass of XmlContainer that declares the items list
plus the srvb:name attribute. ServiceBinding.__init__ now sets
services.name = name when wiring the create body, and the GET
deserialiser populates it from the response.
The skill add-support-for-new-adt-object-type is explicit on this:
"Never change or deduce the XML structure. You can only update the
values of the nodes and attributes to match the new object." The
original fixtures were generated from Marshal().serialize() output
of the partial implementation, which is exactly the deduce-structure
anti-pattern the skill bans. This patch closes the gap.
…tion Some objects like Service Definition are handled by "ADT" but returns URL outside ADT ICF path. I am not proud of this hack but it is too late and I need to push the solution.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/test_sap_cli_srvb.py`:
- Around line 112-114: The headers parameter in the Response() function call is
under-indented, violating Flake8 E128 (continuation line under-indented).
Increase the indentation of the line containing headers={'Content-Type': to
properly align it with the function arguments in the Response() call, ensuring
it follows consistent continuation line indentation rules.
- Line 102: Remove the incorrect .call_args accessor from the mock assertion in
the line with fake_srvb.return_value.add_service. The assert_called_once_with()
method should be called directly on the add_service mock object itself, not on
its call_args property. Change
fake_srvb.return_value.add_service.call_args.assert_called_once_with(...) to
fake_srvb.return_value.add_service.assert_called_once_with(...) while keeping
the same arguments (SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 694a4e2d-0535-40fc-ab96-c46150ab1d67
📒 Files selected for processing (6)
sap/adt/businessservice.pysap/adt/core.pysap/cli/srvb.pytest/unit/fixtures_adt_businessservice.pytest/unit/test_sap_adt_businessservice.pytest/unit/test_sap_cli_srvb.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/fixtures_adt_businessservice.py
- sap/cli/srvb.py
| with patch_get_print_console_with_buffer(): | ||
| args.execute(fake_conn, args) | ||
|
|
||
| fake_srvb.return_value.add_service.call_args.assert_called_once_with(SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=py '\.call_args\.assert_called_once_with\('
python - <<'PY'
from unittest.mock import Mock
m = Mock()
m("x")
print(hasattr(m.call_args, "assert_called_once_with"))
PYRepository: jfilak/sapcli
Length of output: 225
Fix mock assertion target in Line 102.
The assertion should be called on the mock method itself, not on call_args. Use fake_srvb.return_value.add_service.assert_called_once_with(...) instead of fake_srvb.return_value.add_service.call_args.assert_called_once_with(...).
Proposed patch
- fake_srvb.return_value.add_service.call_args.assert_called_once_with(SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002')
+ fake_srvb.return_value.add_service.assert_called_once_with(SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fake_srvb.return_value.add_service.call_args.assert_called_once_with(SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002') | |
| fake_srvb.return_value.add_service.assert_called_once_with(SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/test_sap_cli_srvb.py` at line 102, Remove the incorrect .call_args
accessor from the mock assertion in the line with
fake_srvb.return_value.add_service. The assert_called_once_with() method should
be called directly on the add_service mock object itself, not on its call_args
property. Change
fake_srvb.return_value.add_service.call_args.assert_called_once_with(...) to
fake_srvb.return_value.add_service.assert_called_once_with(...) while keeping
the same arguments (SERVICE_BINDING_NAME, 'ZSAPCLI_TEST_SRVD', '0002').
Naive implementation of parsing Service Group of Service Binding. It is not needed for anything else just to get the published URL so we can magically allow agents to fetch the data.
filak-sap
force-pushed
the
add_srvb_srvd_crud
branch
from
93230af to
d89af98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sap/cli/srvb.py (1)
208-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo: "Biding" should be "Binding".
Three error messages contain the typo "Biding" instead of "Binding" in user-facing output.
✏️ Proposed fix
- raise sap.errors.SAPCliError(f'Business Service Biding {args.binding_name} does not contain any services') + raise sap.errors.SAPCliError(f'Business Service Binding {args.binding_name} does not contain any services')- f'Business Service Biding {args.binding_name} contains more than one Service Definition; ' + f'Business Service Binding {args.binding_name} contains more than one Service Definition; '- f'Business Service Biding {args.binding_name} has no Service Definition ' + f'Business Service Binding {args.binding_name} has no Service Definition 'Also applies to: 212-212, 222-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/cli/srvb.py` at line 208, Fix the spelling typo in three user-facing error messages in the file where "Biding" is incorrectly used instead of "Binding". Locate all three occurrences of the typo in the sap.errors.SAPCliError exception messages (at lines 208, 212, and 222) and replace each instance of "Biding" with "Binding" to correct the spelling in the error output messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/system/test_cases/60-publish_service_binding_odata_v2_ui.sh`:
- Line 7: In the file 60-publish_service_binding_odata_v2_ui.sh, the _tcn
variable is set to 61 but should be set to 60 to match the test case file name.
Change the _tcn variable assignment from 61 to 60 so that generated object names
correctly correspond to test case 60 and prevent naming collisions with adjacent
system tests.
---
Duplicate comments:
In `@sap/cli/srvb.py`:
- Line 208: Fix the spelling typo in three user-facing error messages in the
file where "Biding" is incorrectly used instead of "Binding". Locate all three
occurrences of the typo in the sap.errors.SAPCliError exception messages (at
lines 208, 212, and 222) and replace each instance of "Biding" with "Binding" to
correct the spelling in the error output messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f16561cd-9b90-4e35-9d6b-81ff0dcb1b08
📒 Files selected for processing (10)
doc/commands/srvb.mdsap/adt/businessservice.pysap/cli/srvb.pytest/system/test_cases/60-publish_service_binding_odata_v2_ui.shtest/system/test_cases/61-publish_service_binding_odata_v2_api.shtest/system/test_cases/62-publish_service_binding_odata_v4_ui.shtest/system/test_cases/63-publish_service_binding_odata_v4_api.shtest/unit/fixtures_adt_businessservice.pytest/unit/test_sap_adt_businessservice.pytest/unit/test_sap_cli_srvb.py
✅ Files skipped from review due to trivial changes (1)
- doc/commands/srvb.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/fixtures_adt_businessservice.py
- test/unit/test_sap_adt_businessservice.py
| set -o errexit | ||
| set -o pipefail | ||
|
|
||
| _tcn="61" |
There was a problem hiding this comment.
Fix test-case identifier mismatch in object naming.
Line 7 uses _tcn="61" in test case 60-*, so generated object names won’t match this case number and may collide with adjacent system tests.
Suggested patch
-_tcn="61"
+_tcn="60"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _tcn="61" | |
| _tcn="60" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/system/test_cases/60-publish_service_binding_odata_v2_ui.sh` at line 7,
In the file 60-publish_service_binding_odata_v2_ui.sh, the _tcn variable is set
to 61 but should be set to 60 to match the test case file name. Change the _tcn
variable assignment from 61 to 60 so that generated object names correctly
correspond to test case 60 and prevent naming collisions with adjacent system
tests.
I would love to prepare a better patch or amend the previous commits because Claude made many assumptions without asking for clarification but I have no energy anymore. This patch just finishes creating Service Bindings. There were problem with Category and Implementation where Category 0 means UI and that requires implementation name (I have no idea what that means). On the contrary Category 1 means API and that does not use implementation. I drop the binding types SQL and INA because I have absolutely no idea how to verify the implementation works - feel free to add it.
Thanks to Michael Hoerisch who suggested to simplify agents life by allowing them to get the published data directly by using sapcli because the connection is already configured and otherwise the agent would have build new complicated HTTP request with authentication. This is the first and minimal version - feel free to add cool stuff like listing of EntitySets or rich arguments.
filak-sap
force-pushed
the
add_srvb_srvd_crud
branch
from
d89af98 to
a0dcc76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/unit/test_sap_adt_businessservice.py (2)
319-330: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueClarify misleading comment.
The comment says "expect self.binding to remain None" but the test asserts
binding.binding.typis None (notbinding.bindingitself). The binding object is now always initialized; only its properties are None.Suggested fix
def test_init_without_typ_version_keeps_binding_unset(self): - # backward compatibility: existing rap binding publish callers pass - # only (connection, name) and expect self.binding to remain None until - # fetch() populates it. + # backward compatibility: existing rap binding publish callers pass + # only (connection, name); self.binding is initialized with None + # properties until fetch() populates them. binding = sap.adt.businessservice.ServiceBinding(Connection(), 'TEST_BINDING')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_adt_businessservice.py` around lines 319 - 330, The comment in the test_init_without_typ_version_keeps_binding_unset method is misleading because it states "expect self.binding to remain None" but the test actually asserts that binding.binding itself is not None, only its properties (typ, version, category) are None. Update the comment to accurately reflect that the binding object is always initialized but remains in an unset state with None values for its properties until fetch() populates them.
153-182: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUnused variable and potential test/fixture mismatch.
Line 156 declares
service_name = 'TEST_SERVICE_2'but it's never used in the test. The test publishesservices[0](version0001) but the status message assertion expects text referencing "TEST_SERVICE_2 with version 0002". Either remove the unused variable or verify the fixture response aligns with the intended test scenario.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_adt_businessservice.py` around lines 153 - 182, In the test_service_published method, the variable service_name is declared but never used. Remove this unused variable assignment on line 156. Additionally, verify that the mocked response for the binding.publish call is consistent with the test assertions, specifically ensuring that the status message fixture (SAMPLE_BINDING_OBJECT_REFERENCE or the mocked response) correctly reflects the expected version and service name in the assertion on lines 179-181.sap/adt/businessservice.py (1)
344-344: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFix redundant
Uniontype hint syntax.The type hint
Union[None | ODataV4ServiceGroup | ODataV2ServiceList]mixesUnionwith PEP 604|syntax. Use one or the other:Suggested fix
- def get_service_group(self, service: ServicesContainer) -> Union[None | ODataV4ServiceGroup | ODataV2ServiceList]: + def get_service_group(self, service: ServicesContainer) -> ODataV4ServiceGroup | ODataV2ServiceList | None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/adt/businessservice.py` at line 344, The return type hint for the get_service_group method is mixing Union syntax with PEP 604 pipe operator syntax. Replace the redundant Union wrapper with just the pipe operator syntax by changing Union[None | ODataV4ServiceGroup | ODataV2ServiceList] to None | ODataV4ServiceGroup | ODataV2ServiceList, removing the Union call entirely since the pipe operators already define the union of types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sap/cli/srvb.py`:
- Around line 260-266: The `.json()` call on the response object in this section
can raise a ValueError if the backend doesn't return valid JSON, which will leak
a traceback in CLI output instead of a proper error message. Wrap the entire
response handling block (where connection.execute is called and .json() is
invoked) in a try-except block that catches ValueError exceptions and raises a
SAPCliError with a descriptive message indicating the backend did not return
valid JSON. This ensures JSON decode failures are handled consistently with the
CLI error handling guidelines.
- Around line 246-263: The URL joining in the metadata fetch and the
preview_fetch function uses string concatenation which can result in double
slashes if service_url already ends with a forward slash. In the metadata fetch
at line 246 where service_url is concatenated with '/$metadata' and in the
preview_fetch function where service_url is concatenated with '/' and
args.entity_set, replace the string concatenation with a proper URL
normalization method or utility function that handles path joining correctly.
This will ensure consistent URL formatting regardless of whether service_url
ends with a slash or not, making the calls work reliably across different
gateway configurations.
---
Nitpick comments:
In `@sap/adt/businessservice.py`:
- Line 344: The return type hint for the get_service_group method is mixing
Union syntax with PEP 604 pipe operator syntax. Replace the redundant Union
wrapper with just the pipe operator syntax by changing Union[None |
ODataV4ServiceGroup | ODataV2ServiceList] to None | ODataV4ServiceGroup |
ODataV2ServiceList, removing the Union call entirely since the pipe operators
already define the union of types.
In `@test/unit/test_sap_adt_businessservice.py`:
- Around line 319-330: The comment in the
test_init_without_typ_version_keeps_binding_unset method is misleading because
it states "expect self.binding to remain None" but the test actually asserts
that binding.binding itself is not None, only its properties (typ, version,
category) are None. Update the comment to accurately reflect that the binding
object is always initialized but remains in an unset state with None values for
its properties until fetch() populates them.
- Around line 153-182: In the test_service_published method, the variable
service_name is declared but never used. Remove this unused variable assignment
on line 156. Additionally, verify that the mocked response for the
binding.publish call is consistent with the test assertions, specifically
ensuring that the status message fixture (SAMPLE_BINDING_OBJECT_REFERENCE or the
mocked response) correctly reflects the expected version and service name in the
assertion on lines 179-181.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce6c8d6d-b054-4af6-a459-ccf85b0fffb1
📒 Files selected for processing (10)
doc/commands/srvb.mdsap/adt/businessservice.pysap/cli/srvb.pytest/system/test_cases/60-publish_service_binding_odata_v2_ui.shtest/system/test_cases/61-publish_service_binding_odata_v2_api.shtest/system/test_cases/62-publish_service_binding_odata_v4_ui.shtest/system/test_cases/63-publish_service_binding_odata_v4_api.shtest/unit/fixtures_adt_businessservice.pytest/unit/test_sap_adt_businessservice.pytest/unit/test_sap_cli_srvb.py
✅ Files skipped from review due to trivial changes (1)
- doc/commands/srvb.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/fixtures_adt_businessservice.py
Comment on lines
+246
to
+263
| metadata = connection.execute('GET', service_group.services.service_url + '/$metadata', complete_url=True).text | ||
| console.printout(metadata) | ||
|
|
||
|
|
||
| @CommandGroupPreview.argument('--service', nargs='?', default=None, | ||
| help="Service name of the binding's services to preview") | ||
| @CommandGroupPreview.argument('entity_set') | ||
| @CommandGroupPreview.argument('binding_name') | ||
| @CommandGroupPreview.command('fetch') | ||
| def preview_fetch(connection, args): | ||
| """Fetch and print entries from the specified entity set of one of the services in a Service Binding.""" | ||
|
|
||
| console = args.console_factory() | ||
| _, service_group = _get_service_with_binding_group(connection, args) | ||
| data = connection.execute( | ||
| 'GET', | ||
| service_group.services.service_url + '/' + args.entity_set, | ||
| accept='application/json', |
There was a problem hiding this comment.
Normalize URL joining for preview requests.
Line 246 and Line 262 append path parts blindly; if service_url already ends with /, preview calls become //..., which is brittle across gateways.
Proposed patch
def preview_metadata(connection, args):
@@
- metadata = connection.execute('GET', service_group.services.service_url + '/$metadata', complete_url=True).text
+ base_url = service_group.services.service_url.rstrip('/')
+ metadata = connection.execute('GET', f'{base_url}/$metadata', complete_url=True).text
@@
def preview_fetch(connection, args):
@@
- data = connection.execute(
+ base_url = service_group.services.service_url.rstrip('/')
+ entity_set = args.entity_set.lstrip('/')
+ data = connection.execute(
'GET',
- service_group.services.service_url + '/' + args.entity_set,
+ f'{base_url}/{entity_set}',
accept='application/json',
complete_url=True
).json()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sap/cli/srvb.py` around lines 246 - 263, The URL joining in the metadata
fetch and the preview_fetch function uses string concatenation which can result
in double slashes if service_url already ends with a forward slash. In the
metadata fetch at line 246 where service_url is concatenated with '/$metadata'
and in the preview_fetch function where service_url is concatenated with '/' and
args.entity_set, replace the string concatenation with a proper URL
normalization method or utility function that handles path joining correctly.
This will ensure consistent URL formatting regardless of whether service_url
ends with a slash or not, making the calls work reliably across different
gateway configurations.
Comment on lines
+260
to
+266
| data = connection.execute( | ||
| 'GET', | ||
| service_group.services.service_url + '/' + args.entity_set, | ||
| accept='application/json', | ||
| complete_url=True | ||
| ).json() | ||
|
|
There was a problem hiding this comment.
Wrap JSON decode failures in SAPCliError.
Line 265 can raise ValueError and leak a traceback in CLI output when backend doesn’t return JSON.
Proposed patch
- data = connection.execute(
+ response = connection.execute(
'GET',
- service_group.services.service_url + '/' + args.entity_set,
+ service_group.services.service_url + '/' + args.entity_set,
accept='application/json',
complete_url=True
- ).json()
+ )
+ try:
+ data = response.json()
+ except ValueError as exc:
+ # Convert decode errors to SAPCliError so CLI shows a clean user-facing message.
+ raise sap.errors.SAPCliError(
+ f'Failed to parse JSON response for entity set "{args.entity_set}"'
+ ) from exc
console.printout(json.dumps(data, indent=2))🧰 Tools
🪛 ast-grep (0.44.0)
[info] 266-266: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data, indent=2)
Note: Security best practice.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sap/cli/srvb.py` around lines 260 - 266, The `.json()` call on the response
object in this section can raise a ValueError if the backend doesn't return
valid JSON, which will leak a traceback in CLI output instead of a proper error
message. Wrap the entire response handling block (where connection.execute is
called and .json() is invoked) in a try-except block that catches ValueError
exceptions and raises a SAPCliError with a descriptive message indicating the
backend did not return valid JSON. This ensures JSON decode failures are handled
consistently with the CLI error handling guidelines.
Source: Coding guidelines
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
3 participants
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.