[OPIK-628] Fix update LLM provider api key endpoint #901
Conversation
|
|
||
| @Builder(toBuilder = true) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) | ||
| @Getter |
There was a problem hiding this comment.
I assume that you just want to fix the lack of proper toString implementation.
You could have maintained the previous approach just by changing @Getter annotation by @Data. The advantage is not requiring a custom toString implementation and just relying on @ToString.Exclude. The downside is having a mutable POJO.
You record approach has the reversed pros and cons.
There was a problem hiding this comment.
@andrescrz Unfortunately no:)
Endpoint was failing with {"code":400,"message":"Unable to process JSON"} on deserialization. What's worse, integration tests were ok for the same payload. But in dev or if you run Opik locally with docker-compose issue was reproducible. I tried to play with toString and encryption, but with no result.
The only thing that helped is when I changed regular Java class to record. I double checked locally and with dev deployment before merging.
There was a problem hiding this comment.
Ok, probably the issue is that there was no proper setter, so it couldn't be converted from Json to Java POJO.
There was a problem hiding this comment.
It's a weird error, Setter doesn't help here too.
And the most disturbing is that our integration tests are passing and can deserialize it properly. Also if you use JsonUtils, then deserialization works fine as well. Issue only happens with dockerised Opik.
BorisTkachenko
deleted the
boryst/OPIK-628-updating-a-provider-api-key-sends-400-error
branch
Details
Fix update LLM provider api key endpoint