Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullPointerException when using SafeGuardAroundAdvisor #1393

Closed
habuma opened this issue Sep 22, 2024 · 1 comment · Fixed by #1505
Closed

NullPointerException when using SafeGuardAroundAdvisor #1393

habuma opened this issue Sep 22, 2024 · 1 comment · Fixed by #1505

Comments

@habuma
Copy link
Member

habuma commented Sep 22, 2024

This may or may not be considered a bug, but I do wonder what the expected behavior should be.

Put simply, when using SafeGuardAroundAdvisor, if the advisor detects a sensitive word in the prompt, it returns a ChatResponse with an empty generations list. Because it's empty, ChatResponse returns null from getResult() (see https://github.com/spring-projects/spring-ai/blob/main/spring-ai-core/src/main/java/org/springframework/ai/chat/model/ChatResponse.java#L77-L79
). And because that returns null, the content() method throws a NullPointerException at https://github.com/spring-projects/spring-ai/blob/main/spring-ai-core/src/main/java/org/springframework/ai/chat/client/DefaultChatClient.java#L393
.

Is this the expected behavior? Seems wrong to me. Perhaps, instead of returning an empty list, SafeGuardAroundAdvisor could return some default response stating that because of safeguards, the prompt can't be generated. And that response could be overridden by providing an alternate statement when creating SafeGuardAroundAdvisor.

Alternatively (and arguably better): Instead of returning an empty list or a canned response, have it throw a more meaningful exception. Say, something like SafeGuardException or some such thing.

I'll be happy to submit a pull request for such a change, but before I do, I want to make sure that I understand what the best behavior should be when encountering a sensitive word.

tzolov added a commit to tzolov/spring-ai that referenced this issue Oct 8, 2024
 - Add customizable failure response instead of empty result
 - Add order parameter for advisor prioritization
 - Implement default values for failure response and order
 - Introduce builder pattern for flexible configuration

 Resolves spring-projects#1393
@tzolov
Copy link
Contributor

tzolov commented Oct 8, 2024

Thank you @habuma

This #1505 address your suggestions. It also allows to configure your own response message or control the advisor order.
There is a companion Builder to help configure those if needed.

markpollack pushed a commit that referenced this issue Oct 8, 2024
 - Add customizable failure response instead of empty result
 - Add order parameter for advisor prioritization
 - Implement default values for failure response and order
 - Introduce builder pattern for flexible configuration

 Resolves #1393
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 a pull request may close this issue.

2 participants