Introduc extension to behavior definitions#162
Conversation
📝 WalkthroughWalkthroughAdds BehaviorDefinition as a dedicated ADT object with template/reference support, ADT common types, XmlTextProperty support in annotations and marshalling, CLI commands (extend, listinterfaces) with docs, and comprehensive unit tests for marshalling, templates, BehaviorDefinition, and CLI behaviors. ChangesBDEF Extend and XML text support
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as BDEF CLI
participant BDef as BehaviorDefinition
participant Conn as Connection
participant SAP as SAP Backend
User->>CLI: bdef extend --base-bdef BASE --interface-bdef INTF
CLI->>BDef: list_interfaces(BASE)
BDef->>Conn: execute(GET interfaces for BASE)
Conn->>SAP: request BASE interfaces
SAP-->>Conn: XML interface list
Conn-->>BDef: XML response
BDef-->>CLI: NamedItemList (interfaces)
CLI->>BDef: extend(name=..., base_bdef=BASE, interface_bdef=INTF)
BDef->>BDef: build ADTTemplate(base_bdef, interface_bdef)
CLI->>BDef: create(corrnr=...)
BDef->>Conn: execute(PUT create with template)
Conn->>SAP: create extended BDEF
SAP-->>Conn: success
Conn-->>BDef: response
BDef-->>CLI: created
CLI-->>User: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/unit/test_sap_adt_common_types.py (1)
54-58: ⚡ Quick winAvoid loop-style assertions in unit tests.
Please rewrite this test with sequential explicit assertions (no list-comprehension iteration) to match the repository’s test style rule.
As per coding guidelines, "Write tests sequentially without loops".
🤖 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_common_types.py` around lines 54 - 58, The test test_named_item_list_iteration uses a list-comprehension to assert item names; change it to explicit sequential assertions: keep constructing result via NamedItemList.from_xml(NAMED_ITEM_LIST_XML), assert the length (e.g., len(result) == 2) and then assert each item name separately with self.assertEqual(result[0].name, 'I_PRODUCTTP_2') and self.assertEqual(result[1].name, 'I_PRODUCTTP_3') so no loop/list-comprehension is used.sap/cli/behaviordefinition.py (1)
53-53: 💤 Low valueConsider simplifying
getattrusage.Since
--interface-bdefis defined withdefault=Nonein line 43-44,getattr(args, 'interface_bdef', None)can be simplified toargs.interface_bdef.♻️ Optional simplification
- interface_bdef = getattr(args, 'interface_bdef', None) + interface_bdef = args.interface_bdef🤖 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/behaviordefinition.py` at line 53, The getattr call is unnecessarily defensive: replace getattr(args, 'interface_bdef', None) with direct attribute access args.interface_bdef in the behaviordefinition logic (the variable/interface_bdef assignment that currently uses getattr) since the argument parser defines --interface-bdef with default=None; update the assignment in the function or scope where interface_bdef is set so it reads args.interface_bdef.
🤖 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_adt_behaviordefinition.py`:
- Around line 170-172: Fix the Flake8 E127 over-indentation by aligning
continuation lines of the BehaviorDefinition instantiations so the hanging
indents line up under the opening parenthesis; for example adjust the bdef =
sap.adt.BehaviorDefinition(conn, name='R_PRODUCTTP_EXT', package='MYPACKAGE',
metadata=metadata) call (and the other occurrences around the same pattern at
the referenced ranges) so each continued argument is indented to the same column
as the first argument after the opening parenthesis rather than being
over-indented.
In `@test/unit/test_sap_adt_marshalling.py`:
- Line 9: Adjust the over-indented continuation line in the import/list where
XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, xml_text are listed
so that the continuation aligns with the opening delimiter or uses a consistent
hanging indent; locate the import or parameter list that includes these symbols
(e.g., the line containing "XmlNodeAttributeProperty, XmlContainer,
XmlListNodeProperty, xml_text") and re-indent the wrapped line to match the
opening bracket or follow the project hanging-indent style.
---
Nitpick comments:
In `@sap/cli/behaviordefinition.py`:
- Line 53: The getattr call is unnecessarily defensive: replace getattr(args,
'interface_bdef', None) with direct attribute access args.interface_bdef in the
behaviordefinition logic (the variable/interface_bdef assignment that currently
uses getattr) since the argument parser defines --interface-bdef with
default=None; update the assignment in the function or scope where
interface_bdef is set so it reads args.interface_bdef.
In `@test/unit/test_sap_adt_common_types.py`:
- Around line 54-58: The test test_named_item_list_iteration uses a
list-comprehension to assert item names; change it to explicit sequential
assertions: keep constructing result via
NamedItemList.from_xml(NAMED_ITEM_LIST_XML), assert the length (e.g.,
len(result) == 2) and then assert each item name separately with
self.assertEqual(result[0].name, 'I_PRODUCTTP_2') and
self.assertEqual(result[1].name, 'I_PRODUCTTP_3') so no loop/list-comprehension
is used.
🪄 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: c5ec1290-a443-4996-9c38-df119070d988
📒 Files selected for processing (12)
doc/commands/bdef.mdsap/adt/__init__.pysap/adt/annotations.pysap/adt/behaviordefinition.pysap/adt/common_types.pysap/adt/marshalling.pysap/adt/objects.pysap/cli/behaviordefinition.pytest/unit/test_sap_adt_behaviordefinition.pytest/unit/test_sap_adt_common_types.pytest/unit/test_sap_adt_marshalling.pytest/unit/test_sap_cli_bdef.py
💤 Files with no reviewable changes (1)
- sap/adt/objects.py
| package='MYPACKAGE', responsible='DEVELOPER') | ||
| bdef = sap.adt.BehaviorDefinition(conn, name='R_PRODUCTTP_EXT', | ||
| package='MYPACKAGE', metadata=metadata) |
There was a problem hiding this comment.
Fix continuation indentation to satisfy Flake8 E127.
These continuation lines are over-indented and will trigger lint failures; align hanging indents with the project’s expected style.
Also applies to: 182-184, 204-206
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 170-170: continuation line over-indented for visual indent
(E127)
[error] 172-172: continuation line over-indented for visual indent
(E127)
🤖 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_behaviordefinition.py` around lines 170 - 172, Fix the
Flake8 E127 over-indentation by aligning continuation lines of the
BehaviorDefinition instantiations so the hanging indents line up under the
opening parenthesis; for example adjust the bdef =
sap.adt.BehaviorDefinition(conn, name='R_PRODUCTTP_EXT', package='MYPACKAGE',
metadata=metadata) call (and the other occurrences around the same pattern at
the referenced ranges) so each continued argument is indented to the same column
as the first argument after the opening parenthesis rather than being
over-indented.
It turned out that some ADT objects are serialized into mixed element attributes and element text. The current marshalling and annotations framework allows only either: - attribute with nested object - element without attributes and with text (because at the time I added it, the only example I had was a named element with text). This patch adds an option to define a property with xml_text whose value will be used for element's text and the element's text will be use set to it.
Prerequisite for behavior extension support. The ADT server returns namedItemList XML when querying BO interfaces assigned to a behavior definition. No existing parser covered this format. Decision: use ElementTree-based parsing (like system.py and abapgit.py) rather than the SAX-based Marshal framework, because NamedItemList is a standalone response format, not an ADTObject with annotations.
Extract BehaviorDefinition into sap/adt/behaviordefinition.py to prepare for extending the class with template support and BO interface listing. Keeping the class in objects.py would further bloat an already large file (1500+ lines). Decision: re-export from sap/adt/__init__.py so all existing imports via sap.adt.BehaviorDefinition continue to work unchanged.
Enable querying BO interfaces assigned to a behavior definition via GET /sap/bc/adt/bo/behaviordefinitions/interfaces?name=<bdef_name>. This is needed to validate user-provided interfaces when creating behavior extensions. Decision: implemented as a static method since it operates on the BDEF basepath and does not require an object instance. Decision: pass accept as a list to connection.execute so the content-type validation works correctly with the response charset suffix (the ADT connection validates each accepted type via startswith).
Adds 'sapcli bdef listinterfaces <name>' which lists BO interfaces assigned to a behavior definition. This command helps users discover valid interface names before creating behavior extensions. Decision: use the @CommandGroup.command() decorator pattern (like package.py, abapclass.py) for standalone commands outside the CommandGroupObjectMaster class. Declined: adding the command inside the define() method - the decorator pattern is cleaner and consistent with how other CLI modules add custom commands beyond CRUD.
Add documentation for the new listinterfaces command that allows users to discover BO interfaces assigned to a behavior definition.
Add ADTTemplateProperty and ADTTemplate classes to represent the adtcore:adtTemplate XML element used when creating behavior extensions. Each property has a key attribute and optional text value. Decision: use standalone data classes with to_xml() methods instead of the annotation-based marshalling framework. The adtcore:adtProperty element requires both an XML attribute (adtcore:key) and text content on the same element, which the current marshalling framework does not support (XmlElementKind.TEXT does not handle attributes, OBJECT does not handle text content).
Add template property and override create() to inject the adtcore:adtTemplate XML element when creating behavior extensions. The template is serialized via string replacement before the packageRef element, following the pattern established by Transaction.create(). Decision: the template property defaults to None. When None, create() behaves identically to the base class, preserving backward compatibility for regular behavior definition creation.
Add --base-bdef and --interface-bdef optional arguments to the 'sapcli bdef create' command. When --base-bdef is provided, the command creates a behavior extension instead of a regular behavior definition by setting the adtcore:adtTemplate in the request body. When --interface-bdef is provided, the command validates that the interface is assigned to the base behavior definition (or its extensions) by calling list_interfaces. Validation uses case- insensitive comparison as specified. Decision: add new command 'extend' to make it super clear how to create new extension to behavior definition.
Document the new --base-bdef and --interface-bdef arguments for the create command, including examples for all three creation scenarios: regular BDEF, extension without interface, and extension with explicit interface assignment.
We know that the ADT XML parser does expect certain order of XML elements, but until now we never needed to put anything before adtcore:packageRef in ADT objects. However, BehaviorDefinition extension must send adtcore:adtTemplate as the first child. I wanted to re-order the dictionary __order__ but then I got idea to solve the problem by redefinition in child type. IMHO that is the cleanest solution.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/common_types.py`:
- Around line 36-57: Wrap the XML parsing in NamedItemList.from_xml with a
try/except that catches ElementTree.ParseError (from xml.etree.ElementTree)
around the ElementTree.fromstring call (and the subsequent parsing logic if
desired) and re-raise a sap.errors.SAPCliError (or an appropriate subclass) with
a clear message including the original exception text; ensure the
sap.errors.SAPCliError symbol is imported where needed so callers receive the
SAPCliError instead of a raw ParseError.
In `@test/unit/test_sap_adt_annotation.py`:
- Around line 7-8: The import continuation line is over-indented causing Flake8
E126; adjust the wrapped import so the continuation aligns with the opening
parenthesis or uses a standard hanging indent. Locate the import that includes
XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer,
XmlListNodeProperty, OrderedClassMembers and reduce the leading spaces on the
continuation line(s) so they align under the first imported name or directly
under the opening parenthesis to satisfy Flake8 E126.
In `@test/unit/test_sap_adt_marshalling.py`:
- Around line 1019-1025: The test currently calls marshal.deserialize(...) with
no element text so it doesn't verify readonly xml_text with deserialize=False;
update the XML passed to marshal.deserialize to include element text (e.g., put
some inner text inside <mock:data>), then assert obj.data.attr_prop ==
'attrvalue' and that obj.data.text_prop remains 'default' to ensure the readonly
text property did not get overwritten; refer to the marshal.deserialize call and
the obj.data.text_prop/attr_prop properties when making this change.
🪄 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: 9a3a733d-8347-4670-bb87-13a331cc2c37
📒 Files selected for processing (13)
doc/commands/bdef.mdsap/adt/__init__.pysap/adt/annotations.pysap/adt/behaviordefinition.pysap/adt/common_types.pysap/adt/marshalling.pysap/adt/objects.pysap/cli/behaviordefinition.pytest/unit/test_sap_adt_annotation.pytest/unit/test_sap_adt_behaviordefinition.pytest/unit/test_sap_adt_common_types.pytest/unit/test_sap_adt_marshalling.pytest/unit/test_sap_cli_bdef.py
💤 Files with no reviewable changes (1)
- sap/adt/objects.py
✅ Files skipped from review due to trivial changes (1)
- doc/commands/bdef.md
🚧 Files skipped from review as they are similar to previous changes (6)
- sap/adt/init.py
- sap/adt/behaviordefinition.py
- sap/adt/annotations.py
- test/unit/test_sap_adt_common_types.py
- sap/adt/marshalling.py
- sap/cli/behaviordefinition.py
| @staticmethod | ||
| def from_xml(xml_content): | ||
| """Parse namedItemList XML into a NamedItemList instance""" | ||
|
|
||
| root = ElementTree.fromstring(xml_content) | ||
|
|
||
| total_count_elem = root.find(f'{XMLNS_NAMEDITEM}totalItemCount') | ||
| total_count = int(total_count_elem.text) if total_count_elem is not None and total_count_elem.text else 0 | ||
|
|
||
| items = [] | ||
| for item_elem in root.findall(f'{XMLNS_NAMEDITEM}namedItem'): | ||
| name_elem = item_elem.find(f'{XMLNS_NAMEDITEM}name') | ||
| desc_elem = item_elem.find(f'{XMLNS_NAMEDITEM}description') | ||
| data_elem = item_elem.find(f'{XMLNS_NAMEDITEM}data') | ||
|
|
||
| name = name_elem.text if name_elem is not None else None | ||
| description = desc_elem.text if desc_elem is not None and desc_elem.text else None | ||
| data = data_elem.text if data_elem is not None and data_elem.text else None | ||
|
|
||
| items.append(NamedItem(name, description, data)) | ||
|
|
||
| return NamedItemList(total_count, items) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for defusedxml usage and other XML parsing patterns
echo "=== Checking for defusedxml usage ==="
rg -n "defusedxml" --type=py
echo -e "\n=== Checking for ElementTree.fromstring usage ==="
rg -n "ElementTree\.fromstring" --type=py
echo -e "\n=== Checking for XML parsing patterns ==="
rg -n "\.parse\(|\.fromstring\(" --type=py -g "sap/**/*.py" | head -20Repository: jfilak/sapcli
Length of output: 777
🏁 Script executed:
#!/bin/bash
# Examine the from_xml method and its callers
echo "=== Context around from_xml in common_types.py ==="
sed -n '26,60p' sap/adt/common_types.py
echo -e "\n=== Check imports in common_types.py ==="
head -20 sap/adt/common_types.py
echo -e "\n=== Find callers of from_xml method ==="
rg -n "from_xml\(" --type=py sap/ -B 2 -A 2Repository: jfilak/sapcli
Length of output: 6513
🏁 Script executed:
#!/bin/bash
# Check error handling and exception patterns in XML parsing
echo "=== Check ElementTree usage with error handling ==="
rg -B 3 -A 3 "ElementTree\.fromstring" --type=py sap/
echo -e "\n=== Check for try/except around XML parsing ==="
rg -B 5 -A 5 "ElementTree" --type=py sap/adt/common_types.pyRepository: jfilak/sapcli
Length of output: 2268
🏁 Script executed:
#!/bin/bash
# Verify if ADT data sources are documented as trusted
echo "=== Check ADT module documentation ==="
head -50 sap/adt/__init__.py
echo -e "\n=== Look for security or trust documentation ==="
find sap/ -name "*.py" -exec grep -l "trust\|security\|untrusted" {} \;Repository: jfilak/sapcli
Length of output: 1817
Add error handling to from_xml() to follow coding guidelines.
The ElementTree.fromstring() call can raise ParseError on malformed XML, but the exception is not caught or converted to SAPCliError. Per coding guidelines, exceptions should be derived from sap.errors.SAPCliError to ensure the CLI entry point properly displays error messages instead of stack traces. Wrap the parsing logic in a try/except block and re-raise XML parsing errors as an appropriate SAPCliError subclass.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 40-40: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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/common_types.py` around lines 36 - 57, Wrap the XML parsing in
NamedItemList.from_xml with a try/except that catches ElementTree.ParseError
(from xml.etree.ElementTree) around the ElementTree.fromstring call (and the
subsequent parsing logic if desired) and re-raise a sap.errors.SAPCliError (or
an appropriate subclass) with a clear message including the original exception
text; ensure the sap.errors.SAPCliError symbol is imported where needed so
callers receive the SAPCliError instead of a raw ParseError.
| XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, \ | ||
| OrderedClassMembers |
There was a problem hiding this comment.
Fix hanging-indent continuation (Flake8 E126).
This wrapped import is over-indented and will fail linting.
Proposed fix
-from sap.adt.annotations import xml_attribute, xml_element, XmlElementKind, XmlNodeProperty, XmlElementProperty, \
- XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, \
- OrderedClassMembers
+from sap.adt.annotations import (
+ xml_attribute, xml_element, XmlElementKind, XmlNodeProperty, XmlElementProperty,
+ XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty,
+ OrderedClassMembers
+)📝 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.
| XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, \ | |
| OrderedClassMembers | |
| from sap.adt.annotations import ( | |
| xml_attribute, xml_element, XmlElementKind, XmlNodeProperty, XmlElementProperty, | |
| XmlAttributeProperty, XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, | |
| OrderedClassMembers | |
| ) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 7-7: continuation line over-indented for hanging indent
(E126)
🤖 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_annotation.py` around lines 7 - 8, The import
continuation line is over-indented causing Flake8 E126; adjust the wrapped
import so the continuation aligns with the opening parenthesis or uses a
standard hanging indent. Locate the import that includes XmlAttributeProperty,
XmlNodeAttributeProperty, XmlContainer, XmlListNodeProperty, OrderedClassMembers
and reduce the leading spaces on the continuation line(s) so they align under
the first imported name or directly under the opening parenthesis to satisfy
Flake8 E126.
| marshal.deserialize('<?xml version="1.0" encoding="UTF-8"?>\n' | ||
| '<mock:readonly_text xmlns:mock="https://example.org/mock">\n' | ||
| '<mock:data mock:attr="attrvalue"/>\n' | ||
| '</mock:readonly_text>', obj) | ||
|
|
||
| self.assertEqual(obj.data.attr_prop, 'attrvalue') | ||
| self.assertEqual(obj.data.text_prop, 'default') |
There was a problem hiding this comment.
Strengthen readonly xml_text deserialization assertion.
The XML input has no text node, so this test can pass without validating deserialize=False behavior. Include text content and assert text_prop remains unchanged.
Proposed fix
- marshal.deserialize('<?xml version="1.0" encoding="UTF-8"?>\n'
- '<mock:readonly_text xmlns:mock="https://example.org/mock">\n'
- '<mock:data mock:attr="attrvalue"/>\n'
- '</mock:readonly_text>', obj)
+ marshal.deserialize('<?xml version="1.0" encoding="UTF-8"?>\n'
+ '<mock:readonly_text xmlns:mock="https://example.org/mock">\n'
+ '<mock:data mock:attr="attrvalue">incoming_text</mock:data>\n'
+ '</mock:readonly_text>', obj)🤖 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_marshalling.py` around lines 1019 - 1025, The test
currently calls marshal.deserialize(...) with no element text so it doesn't
verify readonly xml_text with deserialize=False; update the XML passed to
marshal.deserialize to include element text (e.g., put some inner text inside
<mock:data>), then assert obj.data.attr_prop == 'attrvalue' and that
obj.data.text_prop remains 'default' to ensure the readonly text property did
not get overwritten; refer to the marshal.deserialize call and the
obj.data.text_prop/attr_prop properties when making this change.
No description provided.