Skip to content

V2 version, like django-orm#2

Merged
Qwizi merged 3 commits intomasterfrom
v2
Nov 20, 2025
Merged

V2 version, like django-orm#2
Qwizi merged 3 commits intomasterfrom
v2

Conversation

@Qwizi
Copy link
Copy Markdown
Owner

@Qwizi Qwizi commented Nov 20, 2025

Summary by CodeRabbit

  • New Features
    • Async SWAPI v2 client with token-based auth, auto-login and timeouts
    • Rich async query/filter API (ORM-like) with pagination, projections and list response wrapper
    • Models for users, products, commissions, accounts, documents and core endpoints with CRUD
    • Commission attributes proxy with partial-attribute save workflow
    • Dynamic dot-accessible data objects with dirty-tracking
    • Core helpers (Q filters) and utility functions (filter parsing, CSV conversion)
    • Comprehensive SWAPI-specific exception types and consolidated public exports

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 20, 2025

Walkthrough

Adds a new async SWAPI v2 client and ORM-like layer: async HTTP client with token auth/auto-refresh, dynamic dot-access wrappers, a Q filter builder, querysets for list/core endpoints, APIModel base plus many endpoint models, utilities, and a custom exception hierarchy.

Changes

Cohort / File(s) Change Summary
Client
src/swapi_client/v2/client.py
New async SWAPIClient with token-based login(), asyncio.Lock-serialized login, automatic login/refresh on 401, request timeout, JSON parsing, HTTP status error mapping, and convenience methods get/post/patch/delete.
Core entry & CoreQuerySet
src/swapi_client/v2/core_init.py, src/swapi_client/v2/queryset_core.py
New Core entrypoint exposing objects() and CoreQuerySet for /api/me-like endpoints with fields/extra_fields/with_relations, path-normalized async get/post/patch/delete, and model materialization.
Exceptions
src/swapi_client/v2/exceptions.py
New exception hierarchy: SWAPIError, SWAPIAuthError, SWAPINotFoundError, SWAPISchemaError, SWAPIValidationError, SWAPIPermissionDenied, SWAPIConnectionError.
Dynamic wrappers
src/swapi_client/v2/dynamic.py
DynamicObject and DynamicList for recursive dot-access wrapping of dicts/lists, with to_dict()/to_list() and readable __repr__.
Utilities
src/swapi_client/v2/utils.py
New helpers parse_filter_key(), is_iterable_but_not_string(), and list_to_csv().
Query/filter layer
src/swapi_client/v2/q.py, src/swapi_client/v2/queryset.py
New Q filter builder (composition, negation, operator map) and SWAPIQuerySet + SWAPIListResponse providing async filtering, exclude, pagination, ordering, projections, CRUD helpers, and internal param building.
Base model
src/swapi_client/v2/models/base.py
New APIModel with dynamic field wrapping, dirty-tracking, objects() factory returning SWAPIQuerySet, save() (POST/PATCH), delete(), payload builders and rehydration via _reload_from_data.
Models package exports
src/swapi_client/v2/models/__init__.py, src/swapi_client/v2/__init__.py
Re-exports: APIModel, many model classes, SWAPIClient, SWAPIQuerySet, SWAPIListResponse, CoreQuerySet, Core, DynamicObject, DynamicList, Q, exception classes, and utils; updates __all__.
Models — account, product, user, document, core
src/swapi_client/v2/models/account_company.py, src/swapi_client/v2/models/account_user.py, src/swapi_client/v2/models/product.py, src/swapi_client/v2/models/user.py, src/swapi_client/v2/models/document.py, src/swapi_client/v2/models/core.py
Many new APIModel subclasses each defining endpoint attributes; CoreModel extends DynamicObject with pk and __repr__.
Models — commission
src/swapi_client/v2/models/commission.py
Adds Commission with AttributesProxy to manage attribute mutations and save_attributes() (PATCH subset), plus many related APIModel classes and endpoints.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant SWAPIClient
    participant LoginLock as AsyncLock
    participant API

    Caller->>SWAPIClient: _request(method, endpoint, ...)
    activate SWAPIClient

    alt token present
        SWAPIClient->>API: request with Authorization
        API-->>SWAPIClient: response
    else token missing or 401
        SWAPIClient->>LoginLock: acquire
        activate LoginLock
        SWAPIClient->>API: POST /_/security/login
        API-->>SWAPIClient: {token: "..."}
        SWAPIClient->>LoginLock: release
        deactivate LoginLock
        SWAPIClient->>API: retry request with Authorization
        API-->>SWAPIClient: response
    end

    alt status >= 400
        SWAPIClient-->>Caller: raise SWAPIError / subclass
    else invalid JSON
        SWAPIClient-->>Caller: raise SWAPISchemaError
    else success
        SWAPIClient-->>Caller: parsed JSON
    end
    deactivate SWAPIClient
Loading
sequenceDiagram
    participant User
    participant APIModel
    participant SWAPIQuerySet
    participant SWAPIClient
    participant API

    User->>APIModel: save()
    activate APIModel
    alt no pk (create)
        APIModel->>SWAPIQuerySet: create(payload)
        SWAPIQuerySet->>SWAPIClient: post(endpoint, json=payload)
        SWAPIClient->>API: POST
        API-->>SWAPIClient: created data
        SWAPIClient-->>SWAPIQuerySet: response data
        APIModel->>APIModel: _reload_from_data(response)
    else has pk & dirty (update)
        APIModel->>SWAPIQuerySet: patch(pk, json=dirty)
        SWAPIQuerySet->>SWAPIClient: patch(...)
        SWAPIClient->>API: PATCH
        API-->>SWAPIClient: updated data
        SWAPIClient-->>SWAPIQuerySet: response data
        APIModel->>APIModel: _reload_from_data(response)
    end
    deactivate APIModel
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

  • Focus areas:
    • src/swapi_client/v2/client.py — login concurrency (Lock), 401 retry logic, timeout and error mapping.
    • src/swapi_client/v2/models/base.py — dynamic wrapping, setattr/getattr hygiene, payload diffing for PATCH.
    • src/swapi_client/v2/queryset.py — filter/operator mapping, param building, projection/pagination semantics.
    • src/swapi_client/v2/models/commission.pyAttributesProxy mutation tracking and save_attributes() correctness.
    • Exception propagation across async boundaries and client/queryset interactions.

Poem

"I nibble keys beneath the moon,
Tokens hum and queries tune.
Fields hop, attributes align,
Async carrots — sweet design.
🐇✨ — proud rabbit signs the v2."

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'V2 version, like django-orm' accurately summarizes the main change: introducing a new v2 API client with Django ORM-like features including QuerySet, Q objects, and model abstractions.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch v2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (13)
src/swapi_client/v2/q.py (1)

103-118: Note the imperfect operator mapping for hasText.

The mapping of "hasText": "notLike" on line 113 is noted in the comment as lacking a perfect equivalent. This could lead to unexpected behavior when negating hasText filters. Ensure this is documented in user-facing documentation or consider raising a warning when this mapping is used.

src/swapi_client/v2/dynamic.py (1)

1-59: Dynamic wrappers look good; just be aware of potential attribute collisions

Implementation of DynamicObject / DynamicList is clean and should work well for nested JSON. Two minor points to keep in mind (no change strictly required now):

  • DynamicObject exposes all keys as attributes. If SWAPI ever returns keys that are not valid identifiers or collide with Python internals (e.g. __dict__, to_dict), dot access will be awkward or ambiguous.
  • to_dict() iterates over self.__dict__, so if you later add internal attributes on DynamicObject itself, they’ll get serialized too. If that happens, consider filtering out keys starting with _ in to_dict().

Otherwise this is solid and composable with the model layer.

src/swapi_client/v2/queryset_core.py (1)

48-64: Fix _build_params return type annotation

_build_params can return None (return params or None), but is annotated as -> Dict[str, Any]. This will confuse type-checkers.

You can tighten this to an explicit optional:

-from typing import Any, Dict, Optional
+from typing import Any, Dict, Optional
@@
-    def _build_params(self) -> Dict[str, Any]:
+    def _build_params(self) -> Optional[Dict[str, Any]]:
         params = {}
@@
-        return params or None
+        return params or None

This aligns the signature with the actual behavior and with the way you pass params through to the client.

src/swapi_client/v2/client.py (3)

135-142: Narrow JSON decode handling and chain the original exception

Catching a bare Exception on resp.json() and re‑raising an unchained SWAPIError is too broad and obscures the root cause.

You can restrict to decode errors and keep the traceback:

-            try:
-                return resp.json()
-            except Exception:
-                raise SWAPIError(
-                    f"Invalid JSON response from SWAPI: {resp.text}"
-                )
+            try:
+                return resp.json()
+            except ValueError as e:
+                # httpx.json() raises ValueError/JSONDecodeError on invalid JSON
+                raise SWAPIError(
+                    f"Invalid JSON response from SWAPI: {resp.text}"
+                ) from e

This satisfies Ruff’s BLE001/B904 guidance and keeps debugging info when the API returns malformed JSON.


147-157: Make Optional argument types explicit in public methods

The signatures

async def get(self, endpoint: str, params: dict = None) -> Dict[str, Any]:
...
async def post(self, endpoint: str, json: dict = None) -> Dict[str, Any]:
...

use implicit Optional (default None but plain dict type), which Ruff flags (RUF013). You can make intent explicit and slightly clearer:

-    async def get(self, endpoint: str, params: dict = None) -> Dict[str, Any]:
+    async def get(self, endpoint: str, params: dict | None = None) -> Dict[str, Any]:
@@
-    async def post(self, endpoint: str, json: dict = None) -> Dict[str, Any]:
+    async def post(self, endpoint: str, json: dict | None = None) -> Dict[str, Any]:
@@
-    async def patch(self, endpoint: str, json: dict = None) -> Dict[str, Any]:
+    async def patch(self, endpoint: str, json: dict | None = None) -> Dict[str, Any]:
@@
-    async def delete(self, endpoint: str, params: dict = None) -> Dict[str, Any]:
+    async def delete(self, endpoint: str, params: dict | None = None) -> Dict[str, Any]:

(Or use Optional[dict] instead if you need to support pre‑3.10 syntax.)


90-132: Consider mapping httpx transport errors to SWAPIConnectionError

_request currently only converts HTTP status failures (status_code >= 400) into SWAPIError. Network-level issues from httpx (ConnectError, ReadTimeout, etc.) will bubble up as raw httpx exceptions, even though you’ve defined SWAPIConnectionError.

Not mandatory, but for a cleaner public API you might want to wrap these:

-    async def _request(self, method: str, endpoint: str, **kwargs) -> Dict[str, Any]:
+    async def _request(self, method: str, endpoint: str, **kwargs) -> Dict[str, Any]:
@@
-        async with httpx.AsyncClient(timeout=self.timeout) as client:
- 
-            # 1) Jeśli pierwszy request → login()
-            if not self.token:
-                async with self._login_lock:
-                    if not self.token:  # drugi request czekał na lock
-                        await self.login()
-
-            # 2) Pierwsze podejście
-            resp = await client.request(
-                method,
-                url,
-                headers=await self._headers(),
-                **kwargs
-            )
+        try:
+            async with httpx.AsyncClient(timeout=self.timeout) as client:
+
+                # 1) Jeśli pierwszy request → login()
+                if not self.token:
+                    async with self._login_lock:
+                        if not self.token:  # drugi request czekał na lock
+                            await self.login()
+
+                # 2) Pierwsze podejście
+                resp = await client.request(
+                    method,
+                    url,
+                    headers=await self._headers(),
+                    **kwargs
+                )
@@
-            # 5) Obsługa błędów
-            if resp.status_code >= 400:
-                raise SWAPIError(
-                    f"SWAPI HTTP {resp.status_code} on {method} {url}: {resp.text}"
-                )
+            # 5) Obsługa błędów
+            if resp.status_code >= 400:
+                raise SWAPIError(
+                    f"SWAPI HTTP {resp.status_code} on {method} {url}: {resp.text}"
+                )
@@
-            try:
-                return resp.json()
-            except Exception:
-                raise SWAPIError(
-                    f"Invalid JSON response from SWAPI: {resp.text}"
-                )
+            try:
+                return resp.json()
+            except ValueError as e:
+                raise SWAPIError(
+                    f"Invalid JSON response from SWAPI: {resp.text}"
+                ) from e
+        except httpx.RequestError as e:
+            # Opcjonalnie: spójny wyjątek dla problemów sieciowych
+            from .exceptions import SWAPIConnectionError
+            raise SWAPIConnectionError(f"Connection error talking to SWAPI: {e}") from e

Exact structure/imports can be adjusted, but the idea is to surface a uniform SWAPIConnectionError for all transport-level failures.

src/swapi_client/v2/models/base.py (3)

39-50: Align _original contents between __init__ and _reload_from_data for stable dirty‑tracking

In __init__ (Line 39–49) _original is populated with the raw data dict, while _reload_from_data (Line 160–169) stores already‑wrapped values (DynamicObject/DynamicList) in _original. That means equality checks in __setattr__ (Line 72–75) compare raw vs wrapped values depending on object lifecycle, which is inconsistent and can produce surprising dirty flags when reassigning dict/list fields.

Consider initializing _original with the same wrapped values you assign to attributes, mirroring _reload_from_data:

@@
-        # surowy JSON ze SWAPI
-        self._raw: Dict[str, Any] = data
-
-       # dynamiczne pola + wrap
-        for key, value in data.items():
-            super().__setattr__(key, self._wrap_dynamic(value))
-
-        # dirty tracking
-        self._original: Dict[str, Any] = dict(data)
-        self._dirty: Dict[str, Any] = {}
+        # surowy JSON ze SWAPI
+        self._raw: Dict[str, Any] = data
+
+        # dirty tracking (przechowujemy wartości po wrapie)
+        self._original: Dict[str, Any] = {}
+        self._dirty: Dict[str, Any] = {}
+
+        # dynamiczne pola + wrap
+        for key, value in data.items():
+            wrapped = self._wrap_dynamic(value)
+            super().__setattr__(key, wrapped)
+            self._original[key] = wrapped

This keeps dirty‑tracking semantics consistent before and after a reload.

Also applies to: 160-169


175-188: __repr__ uses _data, which is never set; consider switching to _raw

__repr__ (Line 175–188) checks and reads self._data, but APIModel only ever sets _raw, _original, and _dirty. As a result, the hasattr(self, "_data") branch is always false and the method always returns a simple <ClassName> repr, never showing pk/fields.

If you want a richer, safe repr, basing it on _raw is more consistent:

@@
     def __repr__(self):
-        # Jeśli obiekt nie ma _data (np. SWAPIListResponse.items jest listą),
-        # to zwracamy prosty repr aby uniknąć błędów.
-        if not hasattr(self, "_data") or not isinstance(self._data, dict):
-            return f"<{self.__class__.__name__}>"
-
-        cls = self.__class__.__name__
-        pk = self._data.get("id") or self._data.get("pk")
-
-        field_names = ", ".join(self._data.keys())
+        raw = getattr(self, "_raw", None)
+        if not isinstance(raw, dict):
+            return f"<{self.__class__.__name__}>"
+
+        cls = self.__class__.__name__
+        pk = raw.get("id") or raw.get("pk")
+
+        field_names = ", ".join(raw.keys())
         if len(field_names) > 120:
             field_names = field_names[:117] + "..."
 
         return f"<{cls} pk={pk} fields={field_names}>"

This keeps the defensive behavior while making the detailed repr actually reachable.


77-85: Clarify exception chaining in __getattr__

In __getattr__ (Line 77–85) you intentionally translate a KeyError into an AttributeError. To make the intent explicit and silence B904, consider using from None to suppress the inner traceback:

-        try:
-            return self.__dict__[item]
-        except KeyError:
-            raise AttributeError(f"{self.__class__.__name__} has no attribute '{item}'")
+        try:
+            return self.__dict__[item]
+        except KeyError:
+            raise AttributeError(
+                f"{self.__class__.__name__} has no attribute '{item}'"
+            ) from None

This keeps user‑visible behavior unchanged while making the traceback cleaner.

src/swapi_client/v2/queryset.py (2)

450-482: Make get() 404 handling more robust and preserve the original exception context

In get() (Line 469–479), 404 detection is based on checking "404" in str(e) and the re‑raised SWAPINotFoundError is not chained:

except SWAPIError as e:
    if "404" in str(e):
        raise SWAPINotFoundError(
            f"{self.model_cls.__name__} id={id} not found"
        )
    raise

This is brittle (depends on the error message formatting) and drops the original traceback.

If the client already distinguishes 404s, prefer raising SWAPINotFoundError there. If you keep this layer doing the mapping, consider:

  • Matching on a structured attribute (e.g. status code) instead of the string, if available.
  • Chaining the exception for easier debugging:
-        except SWAPIError as e:
-            if "404" in str(e):
-                raise SWAPINotFoundError(
-                    f"{self.model_cls.__name__} id={id} not found"
-                )
-            raise
+        except SWAPIError as e:
+            if "404" in str(e):
+                raise SWAPINotFoundError(
+                    f"{self.model_cls.__name__} id={id} not found"
+                ) from e
+            raise

Please verify what information SWAPIError carries today (e.g. HTTP status) so you can avoid relying on string parsing.


529-542: Preserve original cause when wrapping errors in meta()

In meta() (Line 536–539) you catch SWAPIError and wrap it in a new SWAPIError with a formatted message, but you don’t chain the original exception:

except SWAPIError as e:
    raise SWAPIError(f"Failed to fetch meta for {self.endpoint}: {e}")

This loses the root stack trace and also triggers B904.

You can keep the nicer message and still expose the original cause:

-        except SWAPIError as e:
-            raise SWAPIError(f"Failed to fetch meta for {self.endpoint}: {e}")
+        except SWAPIError as e:
+            raise SWAPIError(
+                f"Failed to fetch meta for {self.endpoint}: {e}"
+            ) from e

That makes debugging upstream transport issues much easier.

src/swapi_client/v2/models/commission.py (2)

87-113: Interaction between Commission.attributes proxy and base APIModel.save()

Commission strips attributes out of the initial payload sent to APIModel.__init__ (Line 90–99) and then installs an AttributesProxy via object.__setattr__ (Line 110–112). That means:

  • Normal dirty‑tracking in APIModel completely ignores changes to attributes (as intended).
  • All attribute persistence goes through save_attributes() (Line 116–165), not save().

This is a good separation of concerns, but it also means:

  • await comm.save() will never persist attribute changes; callers must always call await comm.save_attributes() explicitly.
  • If someone manually constructs a Commission with pre‑populated attributes and calls save() for a create, those attributes will be ignored and only later managed through the proxy.

If that’s intentional, consider documenting it more explicitly in the class docstring (e.g. “attributes are only saved via save_attributes()”). Otherwise, you may want to add guardrails (like raising if both attribute proxy dirty state and _dirty are non‑empty during save()).

Also applies to: 116-165


1-10: Minor docs / endpoint consistency issues

A few small inconsistencies that might confuse users:

  • The Commission usage example (Line 69–75) imports from swapi_sdk, while the actual package seems to be swapi_client. Consider aligning the import path.
  • The docstring mentions “dodatkowe metody domenowe: change_phase(), assign_user(), close(), reopen()” (Line 4–10), but these methods are not defined on Commission. Either implement or remove them from the docs to avoid surprises.
  • CommissionUsers.endpoint is set to "/api/commissions_user_userss" (Line 258–258). The double s in userss looks like a potential typo; please confirm against the real SW API schema.

These are non‑blocking but worth cleaning up for clarity.

Also applies to: 69-85, 253-259

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62e9012 and e74747b.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • src/swapi_client/v2/client.py (1 hunks)
  • src/swapi_client/v2/core_init.py (1 hunks)
  • src/swapi_client/v2/dynamic.py (1 hunks)
  • src/swapi_client/v2/exceptions.py (1 hunks)
  • src/swapi_client/v2/models/account_company.py (1 hunks)
  • src/swapi_client/v2/models/account_user.py (1 hunks)
  • src/swapi_client/v2/models/base.py (1 hunks)
  • src/swapi_client/v2/models/commission.py (1 hunks)
  • src/swapi_client/v2/models/core.py (1 hunks)
  • src/swapi_client/v2/models/document.py (1 hunks)
  • src/swapi_client/v2/models/product.py (1 hunks)
  • src/swapi_client/v2/models/user.py (1 hunks)
  • src/swapi_client/v2/q.py (1 hunks)
  • src/swapi_client/v2/queryset.py (1 hunks)
  • src/swapi_client/v2/queryset_core.py (1 hunks)
  • src/swapi_client/v2/utils.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
src/swapi_client/v2/core_init.py (3)
src/swapi_client/v2/models/core.py (3)
  • CoreModel (6-32)
  • Core (35-52)
  • objects (49-52)
src/swapi_client/v2/queryset_core.py (1)
  • CoreQuerySet (7-145)
src/swapi_client/v2/models/base.py (1)
  • objects (97-104)
src/swapi_client/v2/models/account_user.py (1)
src/swapi_client/v2/models/base.py (1)
  • APIModel (24-188)
src/swapi_client/v2/models/user.py (1)
src/swapi_client/v2/models/base.py (1)
  • APIModel (24-188)
src/swapi_client/v2/models/account_company.py (1)
src/swapi_client/v2/models/base.py (1)
  • APIModel (24-188)
src/swapi_client/v2/models/core.py (3)
src/swapi_client/v2/dynamic.py (1)
  • DynamicObject (1-35)
src/swapi_client/v2/queryset_core.py (1)
  • CoreQuerySet (7-145)
src/swapi_client/v2/core_init.py (2)
  • Core (5-22)
  • objects (19-22)
src/swapi_client/v2/queryset_core.py (4)
src/swapi_client/v2/dynamic.py (1)
  • DynamicObject (1-35)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/queryset.py (5)
  • fields (151-153)
  • extra_fields (155-157)
  • with_relations (132-134)
  • _build_params (333-379)
  • get (450-482)
src/swapi_client/v2/client.py (4)
  • get (147-148)
  • post (150-151)
  • patch (153-154)
  • delete (156-157)
src/swapi_client/v2/models/product.py (1)
src/swapi_client/v2/models/base.py (1)
  • APIModel (24-188)
src/swapi_client/v2/models/document.py (1)
src/swapi_client/v2/models/base.py (1)
  • APIModel (24-188)
src/swapi_client/v2/models/commission.py (4)
src/swapi_client/v2/models/base.py (2)
  • APIModel (24-188)
  • pk (90-91)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/client.py (2)
  • get (147-148)
  • patch (153-154)
src/swapi_client/v2/queryset.py (1)
  • get (450-482)
src/swapi_client/v2/queryset.py (4)
src/swapi_client/v2/dynamic.py (1)
  • DynamicObject (1-35)
src/swapi_client/v2/exceptions.py (2)
  • SWAPIError (1-6)
  • SWAPINotFoundError (17-21)
src/swapi_client/v2/queryset_core.py (6)
  • with_relations (41-43)
  • fields (33-35)
  • extra_fields (37-39)
  • get (80-97)
  • _build_params (48-63)
  • post (102-114)
src/swapi_client/v2/client.py (2)
  • get (147-148)
  • post (150-151)
src/swapi_client/v2/models/base.py (4)
src/swapi_client/v2/queryset.py (2)
  • SWAPIQuerySet (56-542)
  • get (450-482)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/dynamic.py (2)
  • DynamicObject (1-35)
  • DynamicList (38-59)
src/swapi_client/v2/client.py (4)
  • post (150-151)
  • get (147-148)
  • patch (153-154)
  • delete (156-157)
src/swapi_client/v2/client.py (4)
src/swapi_client/v2/exceptions.py (2)
  • SWAPIError (1-6)
  • SWAPIAuthError (9-15)
src/swapi_client/v2/queryset_core.py (4)
  • post (102-114)
  • get (80-97)
  • patch (119-131)
  • delete (136-145)
src/swapi_client/v2/queryset.py (1)
  • get (450-482)
src/swapi_client/v2/models/base.py (1)
  • delete (140-144)
🪛 Ruff (0.14.5)
src/swapi_client/v2/core_init.py

21-21: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/models/core.py

51-51: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/queryset_core.py

94-94: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


94-94: Avoid specifying long messages outside the exception class

(TRY003)


111-111: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


111-111: Avoid specifying long messages outside the exception class

(TRY003)


128-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


128-128: Avoid specifying long messages outside the exception class

(TRY003)


143-143: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


143-143: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/models/commission.py

5-5: Docstring contains ambiguous (EN DASH). Did you mean - (HYPHEN-MINUS)?

(RUF002)


50-52: Avoid specifying long messages outside the exception class

(TRY003)


132-132: Avoid specifying long messages outside the exception class

(TRY003)


135-135: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/queryset.py

61-76: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


77-77: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


117-117: Comment contains ambiguous (EN DASH). Did you mean - (HYPHEN-MINUS)?

(RUF003)


124-127: Avoid specifying long messages outside the exception class

(TRY003)


195-195: Avoid specifying long messages outside the exception class

(TRY003)


255-255: Avoid specifying long messages outside the exception class

(TRY003)


476-478: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


476-478: Avoid specifying long messages outside the exception class

(TRY003)


521-521: Avoid specifying long messages outside the exception class

(TRY003)


539-539: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


539-539: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/models/base.py

84-84: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


84-84: Avoid specifying long messages outside the exception class

(TRY003)


99-99: Avoid specifying long messages outside the exception class

(TRY003)


117-117: Avoid specifying long messages outside the exception class

(TRY003)


142-142: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/client.py

64-66: Avoid specifying long messages outside the exception class

(TRY003)


71-71: Avoid specifying long messages outside the exception class

(TRY003)


130-132: Avoid specifying long messages outside the exception class

(TRY003)


139-139: Do not catch blind exception: Exception

(BLE001)


140-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


140-142: Avoid specifying long messages outside the exception class

(TRY003)


147-147: PEP 484 prohibits implicit Optional

Convert to T | None

(RUF013)


150-150: PEP 484 prohibits implicit Optional

Convert to T | None

(RUF013)


153-153: PEP 484 prohibits implicit Optional

Convert to T | None

(RUF013)


156-156: PEP 484 prohibits implicit Optional

Convert to T | None

(RUF013)

🔇 Additional comments (8)
src/swapi_client/v2/models/document.py (1)

5-10: LGTM! Clean model definition.

The DocumentSeriesDefinition model follows the established APIModel pattern with a clear endpoint declaration. All CRUD operations, dirty tracking, and queryset functionality are inherited from the base class.

src/swapi_client/v2/models/account_user.py (1)

3-25: LGTM! Well-structured model definitions.

All three models (AccountUser, AccountUserAttribute, AccountUserHistory) correctly follow the APIModel pattern with properly defined endpoints. The structure is consistent with other model files in the v2 package.

src/swapi_client/v2/utils.py (1)

8-41: LGTM! Clean utility functions for Django-style filtering.

The utility functions are well-implemented for their intended purpose:

  • parse_filter_key correctly handles nested field paths with operators
  • is_iterable_but_not_string appropriately checks for collections used in 'in'/'notIn' operators
  • list_to_csv provides simple CSV conversion suitable for query parameters
src/swapi_client/v2/q.py (1)

18-99: LGTM! Solid Django-style Q object implementation.

The Q object implementation correctly handles:

  • Leaf filters with field__operator=value syntax
  • Logical composition via & (AND) and | (OR)
  • Negation via ~ operator
  • Proper tree structure with children and connectors

The implementation provides a clean, intuitive API for building complex filters.

src/swapi_client/v2/models/account_company.py (1)

4-25: LGTM! Consistent model definitions.

All three account company models (AccountCompany, AccountCompanyAttribute, AccountCompanyHistory) are well-structured and follow the established APIModel pattern. The implementation mirrors the account_user models appropriately.

src/swapi_client/v2/models/user.py (1)

3-49: LGTM! Comprehensive user model definitions.

All six user-related models are properly structured with clear endpoints:

  • User, UserAbsence, UserAbsenceLimit, UserAttribute, UserHistory, UserProfile

Each model correctly inherits from APIModel and provides appropriate REST endpoints. The implementation is consistent with the broader v2 models suite.

src/swapi_client/v2/exceptions.py (1)

1-61: Exception hierarchy is clear and cohesive

The exception tree is well-structured (SWAPIError base + specific subclasses) and maps cleanly to typical HTTP/API failure modes. This should make downstream error handling straightforward.

src/swapi_client/v2/models/product.py (1)

2-59: Product model endpoints are consistent and minimal

The product-related APIModel subclasses correctly follow the pattern from APIModel (only setting endpoint). As long as the backend paths are accurate, this is a clean, declarative mapping.

Comment thread src/swapi_client/v2/core_init.py Outdated
Comment thread src/swapi_client/v2/models/base.py
Comment thread src/swapi_client/v2/models/commission.py
Comment thread src/swapi_client/v2/models/core.py Outdated
Comment thread src/swapi_client/v2/models/core.py Outdated
Comment thread src/swapi_client/v2/queryset_core.py
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/swapi_client/v2/__init__.py (1)

30-51: Consider sorting __all__ alphabetically (optional).

The linter suggests alphabetically sorting __all__. While the current logical grouping (client/query, exceptions, utils) is readable, alphabetical sorting improves consistency and makes it easier to spot missing exports at scale.

Apply this diff if you prefer linter-compliant alphabetical ordering:

 __all__ = [
-	"SWAPIClient",
-	"Q",
-	"SWAPIQuerySet",
-	"SWAPIListResponse",
 	"CoreQuerySet",
 	"DynamicObject",
 	"DynamicList",
-	"models",
-	# exceptions
-	"SWAPIError",
+	"Q",
 	"SWAPIAuthError",
+	"SWAPIConnectionError",
+	"SWAPIError",
+	"SWAPIListResponse",
 	"SWAPINotFoundError",
-	"SWAPISchemaError",
-	"SWAPIValidationError",
 	"SWAPIPermissionDenied",
-	"SWAPIConnectionError",
-	# utils
-	"parse_filter_key",
+	"SWAPIQuerySet",
+	"SWAPISchemaError",
+	"SWAPIValidationError",
+	"SWAPIClient",
 	"is_iterable_but_not_string",
 	"list_to_csv",
+	"models",
+	"parse_filter_key",
 ]
src/swapi_client/v2/models/__init__.py (2)

16-16: Consider splitting the long import line for readability (optional).

Line 16 imports seven product-related models on a single line, making it harder to scan. Splitting into multiple lines would improve readability.

Apply this diff:

-from .product import Product, ProductAttribute, ProductCategory, ProductTemplate, ProductToProductCategory, ServicedProduct, ServicedProductAttribute
+from .product import (
+    Product,
+    ProductAttribute,
+    ProductCategory,
+    ProductTemplate,
+    ProductToProductCategory,
+    ServicedProduct,
+    ServicedProductAttribute,
+)

20-51: Consider sorting __all__ alphabetically (optional).

With 31 exports, alphabetical sorting would make it easier to verify completeness and spot duplicates.

Apply this diff for alphabetical ordering:

 __all__ = [
-    "APIModel",
     "AccountCompany",
     "AccountCompanyAttribute",
     "AccountCompanyHistory",
     "AccountUser",
     "AccountUserAttribute",
     "AccountUserHistory",
+    "APIModel",
     "Commission",
     "CommissionAttribute",
     "CommissionHistory",
     "CommissionPhase",
     "CommissionScopeType",
     "CommissionShortcut",
     "CommissionTemplate",
     "CommissionUsers",
     "CoreModel",
     "DocumentSeriesDefinition",
     "Product",
     "ProductAttribute",
     "ProductCategory",
     "ProductTemplate",
     "ProductToProductCategory",
     "ServicedProduct",
     "ServicedProductAttribute",
     "User",
+    "UserAbsence",
+    "UserAbsenceLimit",
     "UserAttribute",
     "UserHistory",
-    "UserAbsence",
-    "UserAbsenceLimit",
     "UserProfile",
 ]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e74747b and 91e651b.

📒 Files selected for processing (2)
  • src/swapi_client/v2/__init__.py (1 hunks)
  • src/swapi_client/v2/models/__init__.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/swapi_client/v2/__init__.py (7)
src/swapi_client/v2/client.py (1)
  • SWAPIClient (9-157)
src/swapi_client/v2/q.py (1)
  • Q (18-98)
src/swapi_client/v2/queryset.py (2)
  • SWAPIQuerySet (56-542)
  • SWAPIListResponse (27-53)
src/swapi_client/v2/queryset_core.py (1)
  • CoreQuerySet (7-145)
src/swapi_client/v2/dynamic.py (2)
  • DynamicObject (1-35)
  • DynamicList (38-59)
src/swapi_client/v2/exceptions.py (7)
  • SWAPIError (1-6)
  • SWAPIAuthError (9-15)
  • SWAPINotFoundError (17-21)
  • SWAPISchemaError (24-31)
  • SWAPIValidationError (34-46)
  • SWAPIPermissionDenied (49-54)
  • SWAPIConnectionError (57-61)
src/swapi_client/v2/utils.py (3)
  • parse_filter_key (8-23)
  • is_iterable_but_not_string (26-33)
  • list_to_csv (36-41)
src/swapi_client/v2/models/__init__.py (2)
src/swapi_client/v2/models/commission.py (2)
  • Commission (63-179)
  • CommissionUsers (253-258)
src/swapi_client/v2/models/user.py (1)
  • User (3-8)
🪛 Ruff (0.14.5)
src/swapi_client/v2/__init__.py

30-51: __all__ is not sorted

Apply an isort-style sorting to __all__

(RUF022)

src/swapi_client/v2/models/__init__.py

20-51: __all__ is not sorted

Apply an isort-style sorting to __all__

(RUF022)

🔇 Additional comments (1)
src/swapi_client/v2/__init__.py (1)

1-28: LGTM!

The imports are well-organized and clearly structured with logical groupings. The module docstring provides helpful usage examples.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
src/swapi_client/v2/__init__.py (1)

31-53: Public API surface is coherent; consider whether you want __all__ sorted

The exported symbols match the imports and give a clean v2 entry-point. Ruff’s RUF022 about sorting __all__ is purely stylistic here; if you prefer semantic grouping (top-level, exceptions, utils), you can either keep this order and adjust lint config, or alphabetize __all__ to satisfy the rule.

src/swapi_client/v2/queryset_core.py (1)

80-97: Guard against non-dict responses in CoreQuerySet.get

SWAPIClient.get() can return any JSON type, but CoreQuerySet.get assumes a dict (resp.get("data")). If someone reuses this for an endpoint that returns a bare list, this will raise an AttributeError.

If you want CoreQuerySet to be safely reusable beyond strictly object-shaped /api/... endpoints, consider a small guard:

-        data = resp.get("data") or resp
+        if isinstance(resp, dict):
+            data = resp.get("data") or resp
+        else:
+            data = resp
         return self.model_cls(data)

Same pattern could be applied to post/patch if you expect any list-shaped responses there.

src/swapi_client/v2/models/base.py (1)

62-76: Dirty-tracking skips new and nested changes – verify this matches intended semantics

Two behavioral edge cases in the current dirty-tracking logic:

  • __setattr__ only tracks keys already present in _original:
    • New attributes added after initialization (key not in self._original) are never added to _dirty, so they will not be included in PATCH payloads.
  • Nested updates on wrapped DynamicObject/DynamicList aren’t tracked:
    • Mutating obj.address.city updates the nested wrapper, but _dirty remains empty, so save()’s PATCH branch early-returns and never persists those changes.

This is fine if the design is “PATCH only existing top-level JSON fields and ignore new/nested field mutations,” but that’s a surprising limitation for an ORM-like API.

If you want PATCH to cover these cases, options include:

  • Treat any assignment to a key not in _original as dirty (so new fields are sent).
  • When a wrapped field is mutated, mark the top-level key as dirty (e.g., via a helper on DynamicObject/DynamicList or an explicit mark_dirty("field") API on the model).

Happy to sketch a concrete approach if you want to extend dirty tracking rather than keep it minimal.

Also applies to: 149-169

src/swapi_client/v2/models/commission.py (1)

5-5: Optional: Replace EN DASH with HYPHEN-MINUS for consistency.

The docstring contains an EN DASH (–) character. For better compatibility and consistency with standard ASCII text, consider using HYPHEN-MINUS (-) instead.

Based on static analysis hints.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91e651b and ef2d198.

📒 Files selected for processing (6)
  • src/swapi_client/v2/__init__.py (1 hunks)
  • src/swapi_client/v2/core_init.py (1 hunks)
  • src/swapi_client/v2/models/base.py (1 hunks)
  • src/swapi_client/v2/models/commission.py (1 hunks)
  • src/swapi_client/v2/models/core.py (1 hunks)
  • src/swapi_client/v2/queryset_core.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/swapi_client/v2/__init__.py (8)
src/swapi_client/v2/client.py (1)
  • SWAPIClient (9-157)
src/swapi_client/v2/q.py (1)
  • Q (18-98)
src/swapi_client/v2/queryset.py (2)
  • SWAPIQuerySet (56-542)
  • SWAPIListResponse (27-53)
src/swapi_client/v2/queryset_core.py (1)
  • CoreQuerySet (7-145)
src/swapi_client/v2/dynamic.py (2)
  • DynamicObject (1-35)
  • DynamicList (38-59)
src/swapi_client/v2/core_init.py (1)
  • Core (7-24)
src/swapi_client/v2/exceptions.py (7)
  • SWAPIError (1-6)
  • SWAPIAuthError (9-15)
  • SWAPINotFoundError (17-21)
  • SWAPISchemaError (24-31)
  • SWAPIValidationError (34-46)
  • SWAPIPermissionDenied (49-54)
  • SWAPIConnectionError (57-61)
src/swapi_client/v2/utils.py (3)
  • parse_filter_key (8-23)
  • is_iterable_but_not_string (26-33)
  • list_to_csv (36-41)
src/swapi_client/v2/queryset_core.py (4)
src/swapi_client/v2/dynamic.py (1)
  • DynamicObject (1-35)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/queryset.py (5)
  • fields (151-153)
  • extra_fields (155-157)
  • with_relations (132-134)
  • _build_params (333-379)
  • get (450-482)
src/swapi_client/v2/client.py (4)
  • get (147-148)
  • post (150-151)
  • patch (153-154)
  • delete (156-157)
src/swapi_client/v2/models/commission.py (3)
src/swapi_client/v2/models/base.py (2)
  • APIModel (24-199)
  • pk (90-91)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/client.py (2)
  • get (147-148)
  • patch (153-154)
src/swapi_client/v2/models/base.py (4)
src/swapi_client/v2/queryset.py (2)
  • SWAPIQuerySet (56-542)
  • get (450-482)
src/swapi_client/v2/exceptions.py (1)
  • SWAPIError (1-6)
src/swapi_client/v2/dynamic.py (4)
  • DynamicObject (1-35)
  • DynamicList (38-59)
  • to_dict (20-32)
  • to_list (53-59)
src/swapi_client/v2/client.py (4)
  • post (150-151)
  • get (147-148)
  • patch (153-154)
  • delete (156-157)
src/swapi_client/v2/models/core.py (2)
src/swapi_client/v2/dynamic.py (1)
  • DynamicObject (1-35)
src/swapi_client/v2/models/base.py (1)
  • pk (90-91)
src/swapi_client/v2/core_init.py (3)
src/swapi_client/v2/queryset_core.py (1)
  • CoreQuerySet (7-145)
src/swapi_client/v2/models/core.py (1)
  • CoreModel (5-28)
src/swapi_client/v2/models/base.py (1)
  • objects (97-104)
🪛 Ruff (0.14.5)
src/swapi_client/v2/__init__.py

31-53: __all__ is not sorted

Apply an isort-style sorting to __all__

(RUF022)

src/swapi_client/v2/queryset_core.py

94-94: Avoid specifying long messages outside the exception class

(TRY003)


111-111: Avoid specifying long messages outside the exception class

(TRY003)


128-128: Avoid specifying long messages outside the exception class

(TRY003)


143-143: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/models/commission.py

5-5: Docstring contains ambiguous (EN DASH). Did you mean - (HYPHEN-MINUS)?

(RUF002)


50-52: Avoid specifying long messages outside the exception class

(TRY003)


140-140: Avoid specifying long messages outside the exception class

(TRY003)


143-143: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/models/base.py

84-84: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


84-84: Avoid specifying long messages outside the exception class

(TRY003)


99-99: Avoid specifying long messages outside the exception class

(TRY003)


117-117: Avoid specifying long messages outside the exception class

(TRY003)


142-142: Avoid specifying long messages outside the exception class

(TRY003)

src/swapi_client/v2/core_init.py

23-23: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (4)
src/swapi_client/v2/models/core.py (1)

11-28: CoreModel pk and __repr__ now align with DynamicObject semantics

pk correctly mirrors id, and __repr__ inspects public attributes via __dict__, fixing the earlier reliance on non-existent _raw/_data while giving a useful field summary.

src/swapi_client/v2/core_init.py (1)

18-24: Core.objects wiring to CoreQuerySet/CoreModel looks good

Client validation plus CoreQuerySet(cls.client, CoreModel) is consistent with the rest of the API and gives a clear failure when client isn’t configured.

src/swapi_client/v2/models/commission.py (2)

54-62: Excellent fix—in-memory view now stays consistent!

The implementation correctly addresses the previous review concern by updating _attributes_by_id immediately (preserving metadata) while still recording changes in _attributes_dirty for the pending PATCH. This ensures that get() and list() reflect pending changes before save_attributes() is called.


98-120: Good design: Attributes handled separately from dynamic fields.

The __init__ method correctly separates attributes from other fields, preventing them from being wrapped as DynamicObject/DynamicList. Using object.__setattr__ for the proxy (line 119) properly bypasses dirty tracking for this internal field.

Comment on lines +140 to +144
async def delete(self) -> None:
if self.pk is None:
raise SWAPIError("Cannot delete object without primary key")

await self.client.delete(f"{self.endpoint}/{self.pk}")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

delete() should validate client like save() does

save() guards against a missing client with a clear RuntimeError, but delete() goes straight to await self.client.delete(...). If client was never configured, this will fail with a less helpful 'NoneType' object has no attribute 'delete'.

Consider mirroring the save() check:

     async def delete(self) -> None:
-        if self.pk is None:
-            raise SWAPIError("Cannot delete object without primary key")
-
-        await self.client.delete(f"{self.endpoint}/{self.pk}")
+        if self.pk is None:
+            raise SWAPIError("Cannot delete object without primary key")
+
+        if self.client is None:
+            raise RuntimeError("Model has no API client assigned")
+
+        await self.client.delete(f"{self.endpoint}/{self.pk}")
📝 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
async def delete(self) -> None:
if self.pk is None:
raise SWAPIError("Cannot delete object without primary key")
await self.client.delete(f"{self.endpoint}/{self.pk}")
async def delete(self) -> None:
if self.pk is None:
raise SWAPIError("Cannot delete object without primary key")
if self.client is None:
raise RuntimeError("Model has no API client assigned")
await self.client.delete(f"{self.endpoint}/{self.pk}")
🧰 Tools
🪛 Ruff (0.14.5)

142-142: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
In src/swapi_client/v2/models/base.py around lines 140 to 144, the delete()
method does not validate that self.client is set and will raise an unclear
AttributeError if client is None; mirror save() by adding a guard that checks if
self.client is None and raise a RuntimeError with a clear message (e.g., "Client
not configured, cannot delete object") before the existing pk check/HTTP delete
call so the error is explicit and consistent with save().

Comment on lines +186 to +199
def __repr__(self):
# Jeśli obiekt nie ma _data (np. SWAPIListResponse.items jest listą),
# to zwracamy prosty repr aby uniknąć błędów.
if not hasattr(self, "_data") or not isinstance(self._data, dict):
return f"<{self.__class__.__name__}>"

cls = self.__class__.__name__
pk = self._data.get("id") or self._data.get("pk")

field_names = ", ".join(self._data.keys())
if len(field_names) > 120:
field_names = field_names[:117] + "..."

return f"<{cls} pk={pk} fields={field_names}>"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

__repr__ references _data, which is never set → always falls back

APIModel.__repr__ checks and reads self._data, but neither __init__ nor _reload_from_data ever define _data (only _raw, _original, _dirty). As a result, hasattr(self, "_data") is always false and you always get a plain <ClassName> repr, losing the intended field/PK summary.

A minimal fix is to base the repr on _raw (or on public attributes) instead:

-    def __repr__(self):
-        # Jeśli obiekt nie ma _data (np. SWAPIListResponse.items jest listą),
-        # to zwracamy prosty repr aby uniknąć błędów.
-        if not hasattr(self, "_data") or not isinstance(self._data, dict):
-            return f"<{self.__class__.__name__}>"
-
-        cls = self.__class__.__name__
-        pk = self._data.get("id") or self._data.get("pk")
-
-        field_names = ", ".join(self._data.keys())
+    def __repr__(self):
+        cls = self.__class__.__name__
+
+        data = getattr(self, "_raw", None)
+        if not isinstance(data, dict):
+            return f"<{cls}>"
+
+        pk = data.get("id") or data.get("pk")
+
+        field_names = ", ".join(data.keys())
         if len(field_names) > 120:
             field_names = field_names[:117] + "..."
 
         return f"<{cls} pk={pk} fields={field_names}>"

Alternatively, you can mirror CoreModel and derive field_names from self.__dict__’s public keys if you prefer to reflect in-memory state rather than last raw JSON.

🤖 Prompt for AI Agents
In src/swapi_client/v2/models/base.py around lines 186 to 199, __repr__
currently checks for self._data which is never set causing it to always return
the simple "<ClassName>" form; update __repr__ to use the existing backing state
(preferably self._raw if present, else fall back to public attributes from
self.__dict__) to compute pk (from raw.get("id") or raw.get("pk") or attribute
'id'/'pk') and to build the comma-joined field list (truncate >120 chars),
preserving the existing fallback behavior when no suitable data is available;
ensure the function only reads existing attributes (no side effects) and keeps
the same string format "<ClassName pk=... fields=...>".

Comment on lines +48 to +52
if attr_id not in self._commission._attributes_by_id:
# Możesz to zmienić na silent ignore lub warning/log.
raise KeyError(
f"Attribute id={attr_id} does not exist on this Commission instance"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Consider using a more specific exception type.

Raising KeyError for attribute validation is semantically correct but might be confused with internal dictionary key errors. Consider using SWAPIError or ValueError for clearer domain-level error signaling.

Apply this diff:

-        if attr_id not in self._commission._attributes_by_id:
-            # Możesz to zmienić na silent ignore lub warning/log.
-            raise KeyError(
-                f"Attribute id={attr_id} does not exist on this Commission instance"
-            )
+        if attr_id not in self._commission._attributes_by_id:
+            # Możesz to zmienić na silent ignore lub warning/log.
+            raise SWAPIError(
+                f"Attribute id={attr_id} does not exist on this Commission instance"
+            )

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.14.5)

50-52: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
In src/swapi_client/v2/models/commission.py around lines 48 to 52, replace the
raised KeyError when an attribute id is missing with a more specific domain
error (prefer SWAPIError if available, otherwise ValueError) to avoid confusion
with internal dict key errors; import SWAPIError from its module at the top of
the file if necessary, update the raise line to use SWAPIError (or ValueError)
with the same message, and run tests/linters to ensure the new exception type is
recognized where this code is consumed.

Comment on lines +109 to +112
self._attributes_by_id = {
int(item["id"]): dict(item)
for item in raw_attrs
if "id" in item
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling for non-integer or missing attribute IDs.

The int(item["id"]) conversion will raise an exception if item["id"] is None, missing, or not convertible to an integer. Consider adding validation or a try-except block to handle malformed data gracefully.

Apply this diff:

         # 3. We własnej strukturze przechowujemy attributes
         self._attributes_by_id = {
-            int(item["id"]): dict(item)
-            for item in raw_attrs
-            if "id" in item
+            int(item["id"]): dict(item)
+            for item in raw_attrs
+            if item.get("id") is not None
         }

Note: This still allows int() to raise ValueError for non-numeric IDs, but at least prevents KeyError and TypeError for missing/None values. Consider wrapping in try-except if you want to silently skip malformed entries.

📝 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
self._attributes_by_id = {
int(item["id"]): dict(item)
for item in raw_attrs
if "id" in item
self._attributes_by_id = {
int(item["id"]): dict(item)
for item in raw_attrs
if item.get("id") is not None
}
🤖 Prompt for AI Agents
In src/swapi_client/v2/models/commission.py around lines 109-112, the dict
comprehension uses int(item["id"]) which can raise KeyError/TypeError for
missing or None ids; replace the comprehension with an explicit loop that first
checks that "id" in item and item["id"] is not None, then attempts to convert to
int inside a try/except catching ValueError and TypeError and skipping (or
logging) malformed entries so only valid integer ids are used as keys.

Comment on lines +165 to +169
for item in new_attrs:
attr_id = item.get("id")
if attr_id is None:
continue
self._attributes_by_id[int(attr_id)] = dict(item)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling for non-integer attribute IDs in response.

Similar to __init__, the int(attr_id) conversion can fail if the API returns malformed data. Consider adding validation or error handling.

Apply this diff:

         for item in new_attrs:
             attr_id = item.get("id")
             if attr_id is None:
                 continue
-            self._attributes_by_id[int(attr_id)] = dict(item)
+            try:
+                self._attributes_by_id[int(attr_id)] = dict(item)
+            except (ValueError, TypeError):
+                # Skip malformed attribute entries
+                continue
📝 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
for item in new_attrs:
attr_id = item.get("id")
if attr_id is None:
continue
self._attributes_by_id[int(attr_id)] = dict(item)
for item in new_attrs:
attr_id = item.get("id")
if attr_id is None:
continue
try:
self._attributes_by_id[int(attr_id)] = dict(item)
except (ValueError, TypeError):
# Skip malformed attribute entries
continue
🤖 Prompt for AI Agents
In src/swapi_client/v2/models/commission.py around lines 165 to 169, the code
calls int(attr_id) without guarding against malformed API data; wrap the
conversion in the same validation used in __init__: check that attr_id is not
None and is a valid integer (e.g. try: idx = int(attr_id) except (ValueError,
TypeError): log or warn about the bad id and continue), and only assign
self._attributes_by_id[idx] = dict(item) when the conversion succeeds so
malformed IDs are skipped instead of raising.

Comment on lines +261 to +267
class CommissionUsers(APIModel):
"""
Model Commission Usership (użytkownik zlecenia).
"""

endpoint = "/api/commissions_user_userss"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Verify endpoint URL—possible typo with double "userss".

The endpoint /api/commissions_user_userss has a double "s" suffix ("userss") which appears unusual. Please verify whether this is the correct API endpoint or if it should be /api/commissions_user_users or /api/commission_users.

Run the following script to check for similar endpoint patterns in the codebase:


🏁 Script executed:

#!/bin/bash
# Search for other endpoints in model files to identify naming patterns
rg -n --type=py 'endpoint = "/api/commission' src/swapi_client/v2/models/

Length of output: 997


Fix typo in endpoint URL at line 266.

The endpoint /api/commissions_user_userss contains a double "s" suffix. Based on the naming pattern used across all other endpoints in this file, it should be /api/commission_users.

🤖 Prompt for AI Agents
In src/swapi_client/v2/models/commission.py around lines 261 to 267, the
CommissionUsers.endpoint value is wrong: it currently reads
"/api/commissions_user_userss" — change this string to the correct endpoint
"/api/commission_users" to match the file's naming pattern and other endpoints;
update the constant in-place and run lint/tests to ensure no other references
need adjustment.

@Qwizi Qwizi merged commit 285b998 into master Nov 20, 2025
1 check passed
@coderabbitai coderabbitai bot mentioned this pull request Feb 18, 2026
Merged
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