Merged
Conversation
Print system information retrieved from /sap/bc/adt/system/information and /sap/bc/adt/core/http/systeminformation. Renaming "systemID" to "SID" because that's what everybody would expect.
For easy checks that user has valid credentials configured for the given endpoint.
Not all execution paths have handler e.g. "sapcli" and without this patch, the process dies with a stacktrace for missing member and that is not a nice user experience.
📝 WalkthroughWalkthroughThe pull request introduces a system information retrieval module for ADT, adds endpoint-filtering functionality to GCTS user credentials, implements a CLI Changes
Sequence DiagramssequenceDiagram
participant Client
participant Connection
participant XMLEndpoint as ADT<br/>(system/information)
participant JSONEndpoint as ADT<br/>(core/http/systeminformation)
Client->>Connection: get_information()
par Concurrent Fetches
Connection->>XMLEndpoint: GET system/information
XMLEndpoint-->>Connection: Atom XML feed
Connection->>JSONEndpoint: GET core/http/systeminformation<br/>(Accept: SAP JSON)
JSONEndpoint-->>Connection: JSON object
and
end
Connection->>Connection: Parse XML entries
Connection->>Connection: Parse JSON entries
Connection->>Connection: Merge by identity<br/>(XML overwrites JSON)
Connection-->>Client: SystemInformation<br/>(merged entries)
sequenceDiagram
participant CLI
participant GCTSCommand as get-credentials
participant Filter as Endpoint Filter
participant Output
CLI->>GCTSCommand: Execute with --endpoint
GCTSCommand->>GCTSCommand: Fetch all credentials
GCTSCommand->>Filter: Canonicalize endpoint<br/>(trim, strip /, lowercase)
Filter->>Filter: Filter credentials by<br/>canonicalized endpoint
Filter->>Filter: Validate credential state<br/>(state != 'false')
alt Match found & valid
Filter-->>GCTSCommand: Filtered credentials
GCTSCommand->>Output: Print matching entries
else No match
Filter-->>GCTSCommand: Empty result
GCTSCommand->>Output: Error: No credentials
else Match but invalid state
Filter-->>GCTSCommand: Error
GCTSCommand->>Output: Error: Invalid state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (4)
test/unit/test_sap_cli_abap.py (1)
160-173: Unused variableconsolecan be prefixed with underscore.The static analysis correctly identifies that
consoleis unpacked but never used. Consider prefixing it to indicate intentional non-use.♻️ Suggested fix
def test_systeminfo_sends_requests(self): """Test that systeminfo sends GET requests to both endpoints""" connection = Connection([RESPONSE_SYSTEM_INFORMATION, RESPONSE_JSON_SYSTEM_INFORMATION]) args = parse_args(['systeminfo']) - console, factory = make_console_factory() + _console, factory = make_console_factory() args.console_factory = factory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_cli_abap.py` around lines 160 - 173, In test_systeminfo_sends_requests rename the unused local variable console (from the tuple unpacking console, factory = make_console_factory()) to _console to indicate intentional non-use; update the unpacking in the test_systeminfo_sends_requests function so it reads _console, factory = make_console_factory() while leaving Connection, parse_args, args, and the subsequent assertions unchanged.sap/cli/gcts.py (1)
112-128: Error message could be more informative when multiple invalid credentials exist.When all matching credentials have
state == 'false', the error message on line 126 only reports the first match's state. If there are multiple matches, users might not realize all credentials are invalid.Consider clarifying this:
♻️ Suggested improvement
if not valid: - raise SAPCliError(f'Credentials for endpoint {endpoint} are not valid: {matching[0]["state"]}') + raise SAPCliError(f'All {len(matching)} credential(s) for endpoint {endpoint} have invalid state')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/cli/gcts.py` around lines 112 - 128, In _filter_credentials_by_endpoint, improve the error when all matching credentials are invalid by reporting either all matching credential states or a clearer summary instead of using matching[0]["state"]; collect the states from matching (e.g., [cred.get("state", "") for cred in matching]) and include them or a count and the canonical endpoint in the SAPCliError message raised when valid is empty so users can see that multiple credentials exist and their states.sap/adt/system.py (1)
53-53: Usedefusedxmlfor safer XML parsing.
ElementTree.fromstring()is flagged as potentially vulnerable to XML attacks (e.g., billion laughs, external entity expansion). While the data comes from authenticated SAP connections, usingdefusedxmlprovides defense-in-depth. Note thatdefusedxmlis not a complete drop-in replacement—it requires adding a new dependency and updating bothsap/adt/system.py(line 53) andsap/adt/abapgit.py(2 instances) for consistency. The returned Element objects are compatible with stdlib ElementTree methods, so the refactor is straightforward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/adt/system.py` at line 53, Replace stdlib XML parsing with defusedxml: change the import and parsing call in sap/adt/system.py where root = ElementTree.fromstring(resp.text) to use defusedxml's ElementTree (e.g., import defusedxml.ElementTree as ET and call ET.fromstring(resp.text)); do the same for the two occurrences in sap/adt/abapgit.py so all XML parsing uses defusedxml. Update the import statements accordingly and ensure returned Element objects are used the same way as before.test/unit/test_sap_adt_system.py (1)
7-17: Remove unused imports.Static analysis correctly identifies unused imports:
Requestfrommock(line 10)SYSTEM_INFORMATION_XMLfromfixtures_adt_system(line 15)JSON_SYSTEM_INFORMATIONfromfixtures_adt_system(line 16)🧹 Proposed fix to remove unused imports
from mock import ( Connection, Response, - Request, ) from fixtures_adt_system import ( RESPONSE_SYSTEM_INFORMATION, RESPONSE_JSON_SYSTEM_INFORMATION, - SYSTEM_INFORMATION_XML, - JSON_SYSTEM_INFORMATION, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_adt_system.py` around lines 7 - 17, Remove the unused imports by deleting Request from the mock import tuple and removing SYSTEM_INFORMATION_XML and JSON_SYSTEM_INFORMATION from the fixtures_adt_system import tuple in test_sap_adt_system.py so only Connection and Response and only RESPONSE_SYSTEM_INFORMATION and RESPONSE_JSON_SYSTEM_INFORMATION remain; update the import lines that reference Request, SYSTEM_INFORMATION_XML, and JSON_SYSTEM_INFORMATION to avoid unused symbol warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sap/adt/system.py`:
- Around line 53-59: The loop that builds entries from ElementTree.fromstring
can raise AttributeError when entry_elem.find(f'{XMLNS_ATOM}id') or
.find(f'{XMLNS_ATOM}title') returns None; update the loop that populates entries
(the for entry_elem in root.findall(...) block that creates
SystemInfoEntry(identity, title)) to defensively check the result of
entry_elem.find(...) before accessing .text, e.g., extract element =
entry_elem.find(...), skip or supply a safe default if element is None, and
optionally log or warn about missing id/title so you don’t call .text on None.
---
Nitpick comments:
In `@sap/adt/system.py`:
- Line 53: Replace stdlib XML parsing with defusedxml: change the import and
parsing call in sap/adt/system.py where root = ElementTree.fromstring(resp.text)
to use defusedxml's ElementTree (e.g., import defusedxml.ElementTree as ET and
call ET.fromstring(resp.text)); do the same for the two occurrences in
sap/adt/abapgit.py so all XML parsing uses defusedxml. Update the import
statements accordingly and ensure returned Element objects are used the same way
as before.
In `@sap/cli/gcts.py`:
- Around line 112-128: In _filter_credentials_by_endpoint, improve the error
when all matching credentials are invalid by reporting either all matching
credential states or a clearer summary instead of using matching[0]["state"];
collect the states from matching (e.g., [cred.get("state", "") for cred in
matching]) and include them or a count and the canonical endpoint in the
SAPCliError message raised when valid is empty so users can see that multiple
credentials exist and their states.
In `@test/unit/test_sap_adt_system.py`:
- Around line 7-17: Remove the unused imports by deleting Request from the mock
import tuple and removing SYSTEM_INFORMATION_XML and JSON_SYSTEM_INFORMATION
from the fixtures_adt_system import tuple in test_sap_adt_system.py so only
Connection and Response and only RESPONSE_SYSTEM_INFORMATION and
RESPONSE_JSON_SYSTEM_INFORMATION remain; update the import lines that reference
Request, SYSTEM_INFORMATION_XML, and JSON_SYSTEM_INFORMATION to avoid unused
symbol warnings.
In `@test/unit/test_sap_cli_abap.py`:
- Around line 160-173: In test_systeminfo_sends_requests rename the unused local
variable console (from the tuple unpacking console, factory =
make_console_factory()) to _console to indicate intentional non-use; update the
unpacking in the test_systeminfo_sends_requests function so it reads _console,
factory = make_console_factory() while leaving Connection, parse_args, args, and
the subsequent assertions unchanged.
🪄 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: 3edd5433-08b2-44db-9c58-2a99d2341fd3
📒 Files selected for processing (10)
doc/commands/gcts.mdsap/adt/system.pysap/cli/_entry.pysap/cli/abap.pysap/cli/gcts.pytest/unit/fixtures_adt_system.pytest/unit/test_sap_adt_system.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_abap.pytest/unit/test_sap_cli_gcts.py
Comment on lines
+53
to
+59
| root = ElementTree.fromstring(resp.text) | ||
|
|
||
| entries = [] | ||
| for entry_elem in root.findall(f'{XMLNS_ATOM}entry'): | ||
| identity = entry_elem.find(f'{XMLNS_ATOM}id').text | ||
| title = entry_elem.find(f'{XMLNS_ATOM}title').text | ||
| entries.append(SystemInfoEntry(identity, title)) |
There was a problem hiding this comment.
Potential AttributeError if XML entry lacks id or title element.
If the server returns an <entry> without an <atom:id> or <atom:title> child, find() returns None and accessing .text will raise AttributeError. Consider adding defensive checks.
🛡️ Suggested defensive handling
for entry_elem in root.findall(f'{XMLNS_ATOM}entry'):
- identity = entry_elem.find(f'{XMLNS_ATOM}id').text
- title = entry_elem.find(f'{XMLNS_ATOM}title').text
- entries.append(SystemInfoEntry(identity, title))
+ id_elem = entry_elem.find(f'{XMLNS_ATOM}id')
+ title_elem = entry_elem.find(f'{XMLNS_ATOM}title')
+ if id_elem is not None and title_elem is not None:
+ entries.append(SystemInfoEntry(id_elem.text, title_elem.text))🧰 Tools
🪛 Ruff (0.15.7)
[error] 53-53: 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 the current code and only fix it if needed.
In `@sap/adt/system.py` around lines 53 - 59, The loop that builds entries from
ElementTree.fromstring can raise AttributeError when
entry_elem.find(f'{XMLNS_ATOM}id') or .find(f'{XMLNS_ATOM}title') returns None;
update the loop that populates entries (the for entry_elem in root.findall(...)
block that creates SystemInfoEntry(identity, title)) to defensively check the
result of entry_elem.find(...) before accessing .text, e.g., extract element =
entry_elem.find(...), skip or supply a safe default if element is None, and
optionally log or warn about missing id/title so you don’t call .text on None.
This was referenced Mar 27, 2026
Merged
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
2 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.