ottoka commented on PR #1111: URL: https://github.com/apache/james-project/pull/1111#issuecomment-1210356023
> > Effectively, have the messages lie about what they actually have in their content byte array. > > No they are supposed to tell what the underlying data is regardless of what they did actually load. > Thanks for the explanation. In this case, I am OK with the necessary changes and will adapt the code accordingly. > > Tests basically want BODY and FULL to be the same thing in regard to findInMailbox(). > > Good catch. Maybe we should just **drop** Body fetch type and use **FULL** instead, which would simplify the code, and avoid some confusions. We don't use it (FetchType.BODY) so it effectively looks like accidental complexity we might want get rid of... > > That would simplify your issue? > On a quick look I saw that BODY and FULL are different things in the Cassandra Mapper when it loads the message into memory. I do wonder what the use case is for having a message with full (potentially large) body but without the headers. BODY is used in a lot of tests, and I cannot really tell if the differenc eis significant there, or if they really do want FULL and are confised by the distinction. In the latter case, I agree it makes sense to drop BODY, but that will be a bigger/different ticket. > > The deleteMessages... tests are doing something strange during AbstractMessageMapper.updateFlag(). They first retrieve messages with fetch type METADATA and the save them via InMemoryMessageMapper.save(), which copies the truncated message and thus winds up with a broken message in the store?? So, this mechanism actually relies on MessageMappers to ignore the fetch type??? > > What you describe looks like a behavior specific to the in-memory implementation (ignoring fetch type, etc..). So ok memory mailbox, implemented for testing purposes, might benefit from a rework... > > > turns out honoring the fetch type breaks MemoryMessageMapperTest (and MemoryMessageWithAttachmentMapperTest), in different ways > > Do we care about memory honoring the fetch type stuff? Ok, it makes it closer to real implementations (cassandra / JPA) meaning catching some issues earlier in the development cycle, but integration tests (here MPT IMAP tests?) still get us covered. While making memory fetch type aware the way Cassandra implem is would be a contribution I would support, I do not know if I would consider it 'essential' > Agreed it is not essential, but it DID catch a possible bug, so I believe it is worth doing. After your explanations I have a better understanding of the issues involved and feel more comfortable in doing the necessary code changes. > > save them via InMemoryMessageMapper.save(), which copies the truncated message and thus winds up with a broken message in the store?? So, this mechanism actually relies on MessageMappers to ignore the fetch type??? > > We could adapt "save" behaviour to actually update message metadata instead of replacing it, if the message is already present? Again, thanks for the clarification. I compared with JPAMessageMapper, which does exactly that, i.e. re-fetch an existing message to update its flags and modseq. So I will do this for InMemoryMessageMapper as well, which should fix the remaining test failures in MemoryMessageMapperTest. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
