quickfixj icon indicating copy to clipboard operation
quickfixj copied to clipboard

Populate `Text(58)` field when `rejectMessageOnUnhandledException=true`

Open mgatny opened this issue 1 year ago • 3 comments

Supersedes #667 refs #639

If rejectMessageOnUnhandledException=true and an unhandled exception is thrown from the fromCallback(), include the exception message in the Text(58) field of the resulting Reject or BusinessMessageReject.

mgatny avatar Aug 02 '24 17:08 mgatny

@mgatny thanks for the PR. The only thing that troubles me is that it might reveal internal information about the application e.g. "Out of memory" or similar. What do you think about adding a prefix like "error: " or "problem: " to the Text field so that users could possibly filter out such messages in their toAdmin() or toApp() callbacks?

chrjohn avatar Aug 05 '24 11:08 chrjohn

@mgatny any comments on my proposal? https://github.com/quickfix-j/quickfixj/pull/851#issuecomment-2268801218

Thank you

chrjohn avatar Nov 23 '24 19:11 chrjohn

@chrjohn here's what I'm thinking:

  1. If we add Text(58) to the generated Reject at all, we need an additional boolean SessionSetting that enables this behavior, and defaults to "N". Otherwise, anyone already using RejectMessageOnUnhandledException will be surprised that Text(58) automcatically gets set on Rejects.
  2. Instead of QF/j adding a prefix to the Text(58), what if we recommend this approach: a. Set RejectMessageOnUnhandledExceptionSetsTextField=Y (or whatever we call the new boolean setting). b. Use a well-known string (i.e. a constant) as the exception message when intentionally throwing an exception to cause a Reject. c. In toAdmin/toApp, compare the Text(58) to your well-known string(s). d. If it's a match, leave the Text(58) on the message. e. Otherwise remove Text(58) from the message.

This would allow users to get exactly what they want in Text(58), but still give them the ability to filter. And it will not change any behavior for anyone already using RejectMessageOnUnhandledException.

mgatny avatar Dec 03 '24 15:12 mgatny