Issue #173 - Message.IsAdmin/IsApp wrong check #174
Conversation
Message.IsAdmin/IsApp checks presence of MsgType in body instead of header. Then extracts field from header (correct).
See the bug connamara#80.
SocketInitiator has different interface than ThreadedSocketAcceptor (interface extended). It does not allow to specify SessionFactory from the outside. It also stores private variables that are newer used (deleted).
Group gets corrupted when parsing message from string and MessageFactory is not set or MessageFactory fails to create typed Group. Parsing fails on the second instance of the group. Grp must be reset to null when the last instance of group was added to fieldMap. Message.SetGroup method: ... // Create a new group! if (msgFactory != null) grp = msgFactory.Create(Message.ExtractBeginString(msgstr), Message.GetMsgType(msgstr), grpNoFld.Tag); //If above failed (shouldn't ever happen), just use a generic Group. //********** grp already contains first instance of group. if (grp == null) grp = new Group(grpNoFld.Tag, grpEntryDelimiterTag, order);
|
Tomas, it looks like this pull request is a combined fix for #80, #173, and #176. In the future, we'd prefer you keep it to one-fix-per-PR, as it makes it easier to review your code. (Sometimes there is good reason to combine bugs, but I don't think this is one of them.) Second, there are no tests here. We need tests to demonstrate that there is an observable issue, and to prevent some fool from breaking your feature in the future. |
|
Grant, first, you are completely right. I did not figured out how to work properly with github when issuing multiple pull requests at one time. Second, I am sorry - we do not create unit tests at our project. The suggested fix is the only way I am able to contribute to QF.net. BR, Tomas. From: Grant Birchmeier [mailto:notifications@github.com] Tomas, it looks like this pull request is a combined fix for #80#80, #173#173, and #176#176. In the future, we'd prefer you keep it to one-fix-per-PR, as it makes it easier to review your code. (Sometimes there is good reason to combine bugs, but I don't think this is one of them.) Second, there are no tests here. We need tests to demonstrate that there is an observable issue, and to prevent some fool from breaking your feature in the future. — |
|
Of the issues covered by this PR, only 80 remains, and the relevant code submission is noted in that issue's comments. Hence, I'm closing this PR. |
Message.IsAdmin/IsApp checks presence of MsgType in body instead of
header. Then extracts field from header (correct).