Implement SocLimiter for supported Renault vehicles
#25757
Merged
Conversation
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
GetLimitSoc, you currently returnErrNotAvailablefor any error fromsocLevelsG; consider propagating the original error (or wrapping it) so callers and logs can distinguish genuine request failures from a vehicle that simply doesn’t support the endpoint. - Adding
SocMinandSocTargetfields directly onto the generickamereon.Responseshifts it toward a catch‑all type; it might be cleaner to define a dedicated response type (or nested struct) for the/ev/soc-levelsendpoint to keep concerns separated and avoid bloating the base response.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GetLimitSoc`, you currently return `ErrNotAvailable` for any error from `socLevelsG`; consider propagating the original error (or wrapping it) so callers and logs can distinguish genuine request failures from a vehicle that simply doesn’t support the endpoint.
- Adding `SocMin` and `SocTarget` fields directly onto the generic `kamereon.Response` shifts it toward a catch‑all type; it might be cleaner to define a dedicated response type (or nested struct) for the `/ev/soc-levels` endpoint to keep concerns separated and avoid bloating the base response.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@andig Could you rerun the Integration job for me? It looks like there was a hiccup during the Playwright dependency installation. Thanks 🙏 |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
AuthDecorator.RoundTrip,Basemay be nil when you wrap the defaulthttp.Client(sincev.Client.Transportis initially nil), so callingrt.Base.RoundTriprisks a panic; consider defaultingBasetohttp.DefaultTransportif it’s nil. - The new auth retry logic in
AuthDecorator.RoundTriponly retries on transport errors, whereas many auth failures come back as HTTP 401/403 responses; you may want to detect those status codes and triggerLogin+ retry as well to preserve the previous behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthDecorator.RoundTrip`, `Base` may be nil when you wrap the default `http.Client` (since `v.Client.Transport` is initially nil), so calling `rt.Base.RoundTrip` risks a panic; consider defaulting `Base` to `http.DefaultTransport` if it’s nil.
- The new auth retry logic in `AuthDecorator.RoundTrip` only retries on transport errors, whereas many auth failures come back as HTTP 401/403 responses; you may want to detect those status codes and trigger `Login` + retry as well to preserve the previous behavior.
## Individual Comments
### Comment 1
<location> `vehicle/renault/kamereon/api.go:35-39` </location>
<code_context>
- req, err := request.New(method, uri+"?"+params.Encode(), body, headers)
- if err == nil {
- err = v.DoJSON(req, &res)
+ v.Client.Transport = &AuthDecorator{
+ Login: v.login,
+ Keys: v.keys,
+ Identity: v.identity,
+ Base: v.Client.Transport,
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against nil base transport when wrapping the client with AuthDecorator
Because `v.Client.Transport` is likely `nil` here (unless set by `request.NewHelper` or elsewhere), `AuthDecorator.Base` will also be `nil`, and `AuthDecorator.RoundTrip` will panic when calling `rt.Base.RoundTrip(req)`. Please default `Base` to `http.DefaultTransport` when the existing transport is `nil` (e.g., in `NewAPI`: `base := v.Client.Transport; if base == nil { base = http.DefaultTransport }`, then pass `base` as `Base`).
</issue_to_address>
### Comment 2
<location> `vehicle/renault/kamereon/auth.go:28` </location>
<code_context>
+ q.Set("country", "DE")
+ req.URL.RawQuery = q.Encode()
+
+ resp, err := rt.Base.RoundTrip(req)
+ if err == nil {
+ return resp, nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Reauthentication retry will likely send an empty body for POST requests
The old code buffered and rewound the request body before retry. Here you call `Login()` and re-use the same `req` in a second `RoundTrip`, but the body has already been consumed, so POST/PUT retries will send an empty body. To preserve behavior, either retry at a level where you still have the original body bytes, or add buffering/rewind support in this decorator (e.g., store the body bytes in a custom `RoundTripper` or via a replayable `request.New`).
</issue_to_address>
### Comment 3
<location> `vehicle/renault/kamereon/auth.go:29-30` </location>
<code_context>
- var res Response
- req, err := request.New(method, uri+"?"+params.Encode(), body, headers)
- if err == nil {
- err = v.DoJSON(req, &res)
+ v.Client.Transport = &AuthDecorator{
</code_context>
<issue_to_address>
**suggestion:** Retry logic only handles transport errors, not HTTP auth failures (e.g. 401/403)
Because reauth only runs when `RoundTrip` returns a non-nil error, 401/403 responses with `resp != nil` and `err == nil` won’t trigger a login, unlike the old `DoJSON` path that treated non-2xx as errors. Please also check `resp.StatusCode` (e.g. 401/403) and invoke the login+retry flow for those cases so expired tokens don’t lead to permanent request failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
AuthDecorator.RoundTrip,resp.StatusCodeis accessed without checking whetherrespis nil orerris non-nil, which can panic on network errors or transport failures; add a nil/err check before inspectingStatusCodeand avoid retry logic when the initial request failed. - In
AuthDecorator.RoundTrip, when you detecthttp.StatusUnauthorizedand decide to re-authenticate, consider closing the first response body before retrying to avoid leaking connections.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthDecorator.RoundTrip`, `resp.StatusCode` is accessed without checking whether `resp` is nil or `err` is non-nil, which can panic on network errors or transport failures; add a nil/err check before inspecting `StatusCode` and avoid retry logic when the initial request failed.
- In `AuthDecorator.RoundTrip`, when you detect `http.StatusUnauthorized` and decide to re-authenticate, consider closing the first response body before retrying to avoid leaking connections.
## Individual Comments
### Comment 1
<location> `vehicle/renault/kamereon/auth.go:19-28` </location>
<code_context>
+ Identity *gigya.Identity
+}
+
+func (rt *AuthDecorator) RoundTrip(req *http.Request) (*http.Response, error) {
+ // Buffer request body for potential retries
+ var (
+ bodyBuffer []byte
+ err error
+ )
+ if req.Body != nil {
+ bodyBuffer, err = io.ReadAll(req.Body)
+ if err != nil {
+ return nil, err
+ }
+ _ = req.Body.Close()
+
+ req.Body = io.NopCloser(bytes.NewReader(bodyBuffer))
+ }
+
+ resp, err := rt.executeRequest(req)
+
+ if resp.StatusCode == http.StatusUnauthorized {
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle nil response and errors from executeRequest before inspecting StatusCode, and close the first response body on retry.
After `resp, err := rt.executeRequest(req)`, `resp` can be nil on transport errors, so reading `resp.StatusCode` may panic. Also, when retrying on 401 you should close `resp.Body` to avoid leaking connections.
You could structure this as:
```go
resp, err := rt.executeRequest(req)
if err != nil || resp == nil {
return resp, err
}
if resp.StatusCode == http.StatusUnauthorized {
resp.Body.Close()
if err := rt.Login(); err != nil {
return nil, err
}
if bodyBuffer != nil {
req.Body = io.NopCloser(bytes.NewReader(bodyBuffer))
}
return rt.executeRequest(req)
}
return resp, nil
```
</issue_to_address>
### Comment 2
<location> `vehicle/renault/kamereon/api.go:145-154` </location>
<code_context>
+ return res.Data.Attributes, err
+}
+
+func (v *API) DoAction(uri string, data any, res any) error {
+ resContainer := map[string]any{
+ "data": res,
+ }
+
+ reqData := map[string]any{
+ "data": data,
+ }
+
+ req, err := request.New(http.MethodPost, uri, request.MarshalJSON(reqData))
+
+ if err != nil {
+ return err
+ }
+
+ err = v.DoJSON(req, &resContainer)
+
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** Decoding into map[string]any will not populate the typed `res` value as intended.
In `DoAction`, wrapping `res` inside `map[string]any` means `encoding/json` will overwrite the `
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
GetLimitSoc, any error from thesoc-levelsAPI call is currently mapped toapi.ErrNotAvailable; consider returning the underlying error when the request itself fails so that genuine communication/auth issues aren’t silently masked as "not available".
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GetLimitSoc`, any error from the `soc-levels` API call is currently mapped to `api.ErrNotAvailable`; consider returning the underlying error when the request itself fails so that genuine communication/auth issues aren’t silently masked as "not available".
## Individual Comments
### Comment 1
<location> `vehicle/renault/provider.go:131-134` </location>
<code_context>
+var _ api.SocLimiter = (*Provider)(nil)
+
+// GetLimitSoc implements the api.SocLimiter interface
+func (v *Provider) GetLimitSoc() (int64, error) {
+ res, err := v.socLevelsG()
+
+ if err == nil && res.SocTarget != nil {
+ return int64(*res.SocTarget), nil
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Preserve the underlying error in GetLimitSoc instead of always returning ErrNotAvailable
If `v.socLevelsG()` returns an error, this implementation always returns `api.ErrNotAvailable`, which masks the actual failure (e.g. network/HTTP/JSON errors) and complicates debugging. Instead, return the underlying error and only use `api.ErrNotAvailable` when `res.SocTarget` is nil, so callers can distinguish real failures from an unsupported feature.
</issue_to_address>
### Comment 2
<location> `vehicle/renault/kamereon/types.go:59-60` </location>
<code_context>
- // position
+}
+
+type Cockpit struct {
+ TotalMileage *int `json:"totalMileage"`
+}
+
</code_context>
<issue_to_address>
**question (bug_risk):** Using *int for TotalMileage may truncate values that were previously represented as float
This was previously a `*float64` (via the shared `attributes` struct) and was returned as a float64 by the provider. Switching the JSON binding to `*int` will silently truncate any fractional mileage the backend might send. If the Kamereon API contract guarantees this field is always an integer, this change is safe; otherwise consider keeping it as `*float64` and updating the provider accordingly to avoid precision loss.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The change of
BatteryStatus.BatteryLevelfrom*intto*float64is likely to break JSON unmarshalling, since the API appears to return an integer field; either keep it as*intand cast where needed or add a custom unmarshaller instead of changing the type. - The
errors.Asusage forrequest.StatusError(e.g.se := new(request.StatusError); errors.As(err, &se)) is incorrect;seshould be declared asvar se *request.StatusErrorand passed aserrors.As(err, &se)to ensure the type match works and the status-code checks behave as intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change of `BatteryStatus.BatteryLevel` from `*int` to `*float64` is likely to break JSON unmarshalling, since the API appears to return an integer field; either keep it as `*int` and cast where needed or add a custom unmarshaller instead of changing the type.
- The `errors.As` usage for `request.StatusError` (e.g. `se := new(request.StatusError); errors.As(err, &se)`) is incorrect; `se` should be declared as `var se *request.StatusError` and passed as `errors.As(err, &se)` to ensure the type match works and the status-code checks behave as intended.
## Individual Comments
### Comment 1
<location> `vehicle/renault/kamereon/api.go:28-36` </location>
<code_context>
-func New(log *util.Logger, keys keys.ConfigServer, identity *gigya.Identity, login func() error) *API {
- return &API{
+func NewAPI(log *util.Logger, keys keys.ConfigServer, identity *gigya.Identity, login func() error) *API {
+ v := &API{
Helper: request.NewHelper(log),
keys: keys,
identity: identity,
login: login,
}
-}
-
-func (v *API) request_(uri string, body io.Reader) (Response, error) {
- params := url.Values{"country": []string{"DE"}}
- headers := map[string]string{
- "content-type": "application/vnd.api+json",
- "x-gigya-id_token": v.identity.Token,
- "apikey": v.keys.APIKey,
- }
- method := http.MethodGet
- if body != nil {
- method = http.MethodPost
+ v.Client.Transport = &AuthDecorator{
+ Login: v.login,
+ Keys: v.keys,
</code_context>
<issue_to_address>
**issue (bug_risk):** Retry semantics changed from generic error-based retry to only retrying on HTTP 401, which alters behavior on network/transport errors.
With the previous `request` flow, *any* error from `request_` triggered `login()` and a retried request (with body rewound). With `AuthDecorator`, retries now only happen on `StatusUnauthorized` (401) in `RoundTrip`, so transport/network errors (DNS, timeouts, TLS, connection reset, or non-401 auth quirks from Kamereon) will not re-auth+retry anymore. If callers depended on the broader retry behavior, this is a regression. Consider either explicitly confirming that only 401 should trigger re-auth, or extending `RoundTrip` to handle additional auth-related error cases to better match the old semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the wakeup logic, the default path only falls back to
WakeUpMy24on HTTP 403, while other places treat 403/404/502 as equivalent unsupported cases; consider aligning the status handling here so that vehicles that return 404/502 onpause-resumealso get the MY24 fallback. - The various status endpoints (
BatteryStatus,HvacStatus,Cockpit,Position) all manually declare inlinestruct{ Data struct{ Attributes T } }wrappers; you could factor out a small generic/typed helper or shared envelope type to reduce repetition and keep the Kamereon response handling consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the wakeup logic, the default path only falls back to `WakeUpMy24` on HTTP 403, while other places treat 403/404/502 as equivalent unsupported cases; consider aligning the status handling here so that vehicles that return 404/502 on `pause-resume` also get the MY24 fallback.
- The various status endpoints (`BatteryStatus`, `HvacStatus`, `Cockpit`, `Position`) all manually declare inline `struct{ Data struct{ Attributes T } }` wrappers; you could factor out a small generic/typed helper or shared envelope type to reduce repetition and keep the Kamereon response handling consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
AuthDecorator.executeRequest, thecontent-typeheader is unconditionally set toapplication/vnd.api+json; consider only setting it when the caller hasn’t already specified a content type to avoid breaking endpoints that may require a different value. WakeUpMy24uses a hard-codedLastSettingsUpdateTimestampvalue; it would be more robust to derive this from the current time (or document why a fixed timestamp is required) to avoid confusing future readers and potential server-side validation issues.- The
SocLevelsendpoint is parsed directly intoSocLevelswhile other similar endpoints useDataEnvelope[T]; double-check whethersoc-levelsalso returns adata/attributesenvelope and, if so, align the parsing with the other endpoints for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthDecorator.executeRequest`, the `content-type` header is unconditionally set to `application/vnd.api+json`; consider only setting it when the caller hasn’t already specified a content type to avoid breaking endpoints that may require a different value.
- `WakeUpMy24` uses a hard-coded `LastSettingsUpdateTimestamp` value; it would be more robust to derive this from the current time (or document why a fixed timestamp is required) to avoid confusing future readers and potential server-side validation issues.
- The `SocLevels` endpoint is parsed directly into `SocLevels` while other similar endpoints use `DataEnvelope[T]`; double-check whether `soc-levels` also returns a `data/attributes` envelope and, if so, align the parsing with the other endpoints for consistency.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
The Kamereon API uses
Correct. Unfortunately, I'm not sure why this specific input was originally chosen or how exactly the endpoint interprets the data. I didn't want to risk making guesses and breaking existing functionality.
This is correct. The |
Member
|
Is this ok to merge? |
Contributor
Author
|
Should be good to go, unless you have any further feedback. |
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.
This implements
GetLimitSocfor Renault vehicles using the API'ssoc-levelsendpoint. Vehicles that don't support the endpoint will keep the current functionality.