Skip to content

fix: API fixes for IEC API - #270

Merged
GuyKh merged 1 commit into
mainfrom
update-api-2026-07
Jul 5, 2026
Merged

fix: API fixes for IEC API#270
GuyKh merged 1 commit into
mainfrom
update-api-2026-07

Conversation

@GuyKh

@GuyKh GuyKh commented Jul 5, 2026

Copy link
Copy Markdown
Owner

No description provided.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix DeviceIn API auth/header handling and expand device models

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Switch DeviceIn requests to use access token and optional reCAPTCHA header.
• Extend device/device-type models to match newly returned API fields.
• Update example usage to consume a device list from the client.
Diagram

graph TD
  ex["example.py"] --> cli["IECClient.get_device_in()"] --> dat["data.get_device_in()"] --> http["commons.send_get_request()"] --> api{{"IEC DeviceIn API"}}
  dat --> dev(("Device models"))
  dat --> dtype(("DeviceType model"))

  subgraph Legend
    direction LR
    _m["Module/Method"] ~~~ _ext{{"External API"}} ~~~ _model(("Model"))
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize reCAPTCHA/header overrides in request helper
  • ➕ Avoids per-endpoint header branching in each data function
  • ➕ Makes it easier to add future endpoint-specific headers consistently
  • ➖ May require broader refactor of commons helpers and call sites
  • ➖ Risk of unintentionally changing behavior for other endpoints
2. Endpoint-specific auth strategy (token selection per endpoint)
  • ➕ Makes explicit which endpoints require access_token vs id_token
  • ➕ Reduces chance of future regressions when endpoints differ
  • ➖ Adds more configuration/branching in the client/data layer
  • ➖ Slightly more maintenance overhead than a simple fix

Recommendation: The PR’s approach (switch to access_token for DeviceIn and add an optional RecaptchaToken header) is a pragmatic fix with minimal surface-area change. If more endpoints begin requiring special headers or different token types, consider centralizing header overrides and/or codifying per-endpoint auth requirements to prevent drift.

Files changed (5) +27 / -10

Enhancement (4) +21 / -8
example.pyUpdate example to fetch devices via get_devices() +3/-3

Update example to fetch devices via get_devices()

• Replaces DeviceIn response handling with a direct list returned by get_devices(). Adjusts selection logic to use the first device from the list when present.

example.py

iec_client.pyExpose recaptcha_token parameter on IECClient.get_device_in() +5/-2

Expose recaptcha_token parameter on IECClient.get_device_in()

• Extends the client method signature to accept an optional recaptcha_token and forwards it to the data layer. Updates docstring to document the new argument.

iec_api/iec_client.py

device.pyExpand Device and CounterDevice fields to match API payload +6/-2

Expand Device and CounterDevice fields to match API payload

• Adds new device-level fields (disconnect/electronic/disconnected status and report status). Extends CounterDevice with last_mr_type_code and device_digit_length fields for more complete meter metadata.

iec_api/models/device.py

device_type.pyAdd electronic/disconnection fields to DeviceType model +7/-1

Add electronic/disconnection fields to DeviceType model

• Adds is_electronic, is_disconnected, and disconnect_reason fields and updates the example payload comment accordingly. Improves alignment with the API’s returned schema.

iec_api/models/device_type.py

Bug fix (1) +6 / -2
data.pyFix DeviceIn auth token and add optional RecaptchaToken header +6/-2

Fix DeviceIn auth token and add optional RecaptchaToken header

• Updates DeviceIn request authorization to use token.access_token instead of token.id_token. Adds an optional recaptcha_token parameter and conditionally injects a RecaptchaToken header for the endpoint.

iec_api/data.py

@qodo-code-review

qodo-code-review Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. CounterDevice decode may fail ✓ Resolved 🐞 Bug ≡ Correctness
Description
CounterDevice now requires lastMrTypeCode and deviceDigitLength with no defaults, so
deserialization will raise if the API (or any older/stored payload) omits these keys. This breaks
get_device_by_device_id() because it decodes the response into Devices -> CounterDevice.
Code

iec_api/models/device.py[R81-83]

+    last_mr_type_code: int = field(metadata=field_options(alias="lastMrTypeCode"))
    connection_size: ConnectionSize = field(metadata=field_options(alias="connectionSize"))
+    device_digit_length: int = field(metadata=field_options(alias="deviceDigitLength"))
Relevance

⭐⭐⭐ High

Team has accepted robustness fixes for deserialization/runtime issues on missing/unsorted API fields
(e.g., post-deserialize sorting) in PR#212.

PR-#212

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The module’s documented example payload for the CounterDevice endpoint omits the newly-required
keys, and the decoding path in data.get_device_by_device_id() goes through
Devices/CounterDevice, so missing keys will cause decode-time exceptions.

iec_api/models/device.py[22-47]
iec_api/models/device.py[72-85]
iec_api/data.py[333-342]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iec_api.models.device.CounterDevice` added two new fields (`lastMrTypeCode`, `deviceDigitLength`) as required (no default). If these keys are missing in the response, mashumaro deserialization will fail and callers like `get_device_by_device_id()` will raise instead of returning data.

## Issue Context
The same module includes an example response for the endpoint that does not include these two keys, indicating the library previously handled payloads without them.

## Fix Focus Areas
- iec_api/models/device.py[72-85]
- iec_api/models/device.py[22-47]
- iec_api/data.py[333-342]

## Suggested fix
Change the new fields to be backward-compatible:
- Make them `Optional[int]` with `default=None` (preferred), OR
- Provide safe defaults if the API semantics require a numeric value.

Example:
- `last_mr_type_code: Optional[int] = field(default=None, metadata=field_options(alias="lastMrTypeCode"))`
- `device_digit_length: Optional[int] = field(default=None, metadata=field_options(alias="deviceDigitLength"))`

This keeps decoding robust when the API omits these fields.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. RecaptchaToken header leaks ✓ Resolved 🐞 Bug ☼ Reliability
Description
data.get_device_in() adds RecaptchaToken into the shared HEADERS_WITH_AUTH dict, so once set
it can persist into later requests even when recaptcha_token is not provided. This contaminates
subsequent calls with a stale/incorrect token and can cause intermittent request failures.
Code

iec_api/data.py[R432-434]

+    headers = commons.add_auth_bearer_to_headers(HEADERS_WITH_AUTH, token.access_token)
+    if recaptcha_token:
+        headers["RecaptchaToken"] = recaptcha_token
Relevance

⭐⭐ Medium

No prior accepted/rejected precedent on mutating shared HEADERS_WITH_AUTH; header-related reviews in
PR#219 were mostly rejected.

PR-#219

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
HEADERS_WITH_AUTH is a shared module-level dict; both add_auth_bearer_to_headers() and the new
recaptcha logic mutate the passed dict, so RecaptchaToken can remain set for subsequent calls.

iec_api/const.py[22-28]
iec_api/commons.py[24-34]
iec_api/data.py[428-438]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iec_api.data.get_device_in()` currently mutates the module-level `HEADERS_WITH_AUTH` by adding a per-request `RecaptchaToken` header. Because the headers dict is shared, the token can persist across subsequent calls (even when `recaptcha_token` is omitted), causing request contamination.

## Issue Context
- `HEADERS_WITH_AUTH` is a module-level dict constant.
- `commons.add_auth_bearer_to_headers(...)` mutates the passed dict in-place.
- `get_device_in()` additionally mutates the same dict by setting `headers["RecaptchaToken"]`.

## Fix Focus Areas
- iec_api/data.py[428-438]
- iec_api/commons.py[24-34]
- iec_api/const.py[22-28]

## Suggested fix
In `get_device_in()`, start from a copy of `HEADERS_WITH_AUTH` (or build a fresh dict) before adding Authorization and `RecaptchaToken`, e.g.:
- `headers = commons.add_auth_bearer_to_headers(HEADERS_WITH_AUTH.copy(), token.access_token)`
- then conditionally add `RecaptchaToken` to that copied dict.
This prevents a previously-set recaptcha token from leaking into later calls.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Optional status defaults to 0 ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Device.report_result_status is Optional[int] but defaults to 0, so missing
reportResultStatus is indistinguishable from an explicit zero returned by the API. This can lead
to incorrect downstream logic that treats 0 as a real status code rather than “not provided”.
Code

iec_api/models/device.py[58]

+    report_result_status: Optional[int] = field(default=0, metadata=field_options(alias="reportResultStatus"))
Relevance

⭐⭐ Medium

No historical evidence on Optional[int] defaulting to 0 vs None semantics in models; similar
model-style suggestions were mixed.

PR-#212

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Device now forces a missing reportResultStatus to 0, while another model (MeterReadingData)
treats the same alias as optional with a None default, indicating the intended semantic
distinction.

iec_api/models/device.py[50-61]
iec_api/models/remote_reading.py[150-156]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Device.report_result_status` is typed as `Optional[int]` but uses `default=0`, which collapses two different states (missing vs explicitly 0). This makes it harder for callers to correctly interpret the API response.

## Issue Context
A similar field in `MeterReadingData` already defaults to `None`, suggesting the project’s convention is to preserve missing-as-None.

## Fix Focus Areas
- iec_api/models/device.py[50-61]
- iec_api/models/remote_reading.py[150-156]

## Suggested fix
Change `report_result_status` default from `0` to `None`:
- `report_result_status: Optional[int] = field(default=None, metadata=field_options(alias="reportResultStatus"))`
This preserves the ability to distinguish “field absent” from “status code 0”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread iec_api/data.py Outdated
Comment thread iec_api/models/device.py Outdated
Comment thread iec_api/models/device.py Outdated
@GuyKh
GuyKh force-pushed the update-api-2026-07 branch from 1af0dc1 to a4036c1 Compare July 5, 2026 08:47
@GuyKh
GuyKh force-pushed the update-api-2026-07 branch from a4036c1 to 3470fea Compare July 5, 2026 08:53
@GuyKh
GuyKh merged commit 6825049 into main Jul 5, 2026
2 checks passed
@GuyKh
GuyKh deleted the update-api-2026-07 branch July 5, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant