Hello David, Quite a long email :) .
I'll try to come up with some answers. From what I recall, the protocols where implemented by Norman Maurer mostly. However you can't see that from the git history since it has been removed with the migration from SVN -> GIT. I do believe that is a big legal issue since code committers retain their copyright and with the migration, we removed any link to them - but it's another discussion. See my reply inline: La 01.07.2020 12:27, David Leangen a scris: > Thanks to those who have replied so far. > > I am going to try to ask better questions. What I am trying to figure out is > how James models the SMTP concepts in the code. I am very particular in that > I think it is extremely important to make the code as “readable” as possible. > I wanted to hear the thoughts behind the current model. > > > I will dive in and tell you what I see, and ask some more questions. :-) > > Please remember that I represent those who are new to James and who want to > use it and understand it. I also want to be able to document it better, which > means that I need to understand it better. > > > I started my journey with investigating the SMTP protocol api and > implementation in James. I am specifically looking at the protocols-smtp > project. Here is the train of thoughts I had as I tried to navigate the code > to figure out how James models smtp. > > > The first place I naturally look is the package > org.apache.james.protocols.smtp. The package seems to be well-named: it is > appropriate and easy to understand. > > Within this package, there are sub-packages: core, dsn, and hook. Those are > definitely not the first place I would look to understand the smtp api. > Within the interfaces / classes in the “main” package (i.e. > “org.apache.james.protocols.smtp”), I find: > > AllButStartTlsLineBasedChannelHandler.java > AllButStartTlsLineChannelHandlerFactory.java > CommandInjectionDetectedException.java > MailAddress.java > MailAddressException.java > MailEnvelope.java > MailEnvelopeImpl.java > SMTPConfiguration.java > SMTPConfigurationImpl.java > SMTPProtocol.java > SMTPProtocolHandlerChain.java > SMTPResponse.java > SMTPRetCode.java > SMTPServerMBean.java > SMTPSession.java > SMTPSessionImpl.java > SMTPStartTlsResponse.java > > Within these, MailAddress and MailAddressException are deprecated, so I > ignore them. > > My first reactions are: > > * AllButStartTlsLineBasedChannelHandler* … wtf??? Ok, don’t want to get into > that right now. > * Why are there impl classes in the main api package???? > * Why is there an MBean class polluting the api??? > * SMTPProtocol looks like the most sensical place to start, so… > > I look at SMTPProtocol. My thoughts are: > > * Holy crap, this is a class, not an interface. What’s going on here?? > * Ok, it implements org.apache.james.protocols.api.ProtocolImpl, which > itself implements org.apache.james.protocols.api.Protocol > - Already this is confusing and there is a lot of coupling going on > here. Arg. > > Ok, first let’s look at the Protocol API. > > Ok, this is an interface, so I am indeed in api territory. I feel a bit > better. But why is the protocols-smtp api coupled to the protocols-api > project? That is not clear. Is there really a benefit to this inheritance? > Coupling apis is generally quite yucky. Is the idea that ALL protocols are > the same? I believe the protocols share the protocols-api beacuse they are all/mostly all line based protocols. Like you, I do think this is not a good solution, we should use composition instead of inheritance. > My thought is that even if there is overlap between the protocol apis, it is > a bit dubious to create this kind of coupling just because it may save a few > lines of code. It has a fairly high risk of creating a world of pain in terms > of causing unnecessary coupling and more difficult dependency management > later on. > > —> I understand that there are “protocols” and that they each “do > something”, but can somebody explain the decision to use this kind of > inheritance to try to define all of the protocols as a single “thing", > instead of having independent apis per protocol? Is there some benefit to > this coupling that I am not seeing? > > Anyway, to continue… Agian, I do think protocols should be independent since they have different rules and share only some technical aspects. The 'business' rules should trump over the technical part, but the code is quite old on that part and it works so I guess no-one put the work to change it much. I believe each protocol is independent and should build to an independent server (SMTP, IMAP, POP3, JMAP) . We could compose these independent servers into a single running process since they share the same language and platform (Java). I do believe some duplication is better than coupling (this also applies to dependency management). > A Protocol has a ProtocolHandlerChain, a ProtocolConfiguration, and a > ProtocolSession. Alrighty, let’s look at those. > > ProtocolHandlerChain - this is in the org.apache.james.protocols.api.handler > package, which means that the “main” package depends on a sub-package. That > seems backwards to me. Anyway, when I look at this interface, I kinda get a > sense of what it does, but it is soooo abstract and there is so little > documentation that really I can only guess as to what it is for. > > ProtocolConfiguration - this is also in the “main” package, which is fine. > This is very abstract, but since it represents a “Configuration” I think > that’s ok. However, the provided documentation is pretty much useless. It > only repeats the name of the interface methods without giving any additional > information as to why these methods exist in the first place. > > ProtocolSession - This opens up a whole new pandora’s box that I am not ready > to investigate right now. I’ll have to try to bite this part of the elephant > later. > Regarding ProtocolHandlerChain, the API is driven by how Netty implements protocols so please read a bit about that https://netty.io/wiki/user-guide-for-4.x.html#wiki-h3-4 . In summary: * Netty implements an interceptor chain. * Each element of that chain is responsible for some processing and builds a more rich data structure for the next element * SMPT/ IMAP protocols are line based and each line is usually a command * You implement a handler for each protocol command, and all the logic is encapsulated there. * Each protocol is essentially a chain of handlers, where each handler will implement a protocol command. I don't believe the code is super clean but it does work and is quite efficient and fast. NOTE: Netty should be upgraded to the latest version, I can do that once we are done with Gradle. > Ok, so back to… ummmm, where was I? I’m already lost, so I’ll have to start > again at SMTPProtocol and try to find my way again. > > > Well, maybe I’ve already said enough for now anyway. I’ll leave it here for > now, await responses, and continue again later. > > > > My incomplete (and maybe unjustified??) conclusions so far: > > * There is a lot of coupling that I don’t understand and that makes the api > already more complex than I would like > * There are many very abstract concepts that make comprehension very > difficult for me > * The api and implementation classes are all mixed together, which I find > utterly confusing > > Not sure if this is helpful or not, but I think that the ability to > comprehend the codebase is very important, especially in a large and complex > system in which many people collaborate. > > Hopefully people can answer some of the questions I pose in my thoughts above. I agree with the coupling and it is going to take some effort to go through all of the modules and clean them up. This can be done as part of the project re-organization that was proposed in https://issues.apache.org/jira/browse/JAMES-3259 Also see https://issues.apache.org/jira/browse/JAMES-2510 . I support those efforts and I would like to do that after we migrate to Gradle which will help with all of the points above. It has much better support for multi module projects. -- Eugen Stan +40720 898 747 / netdava.com
<<attachment: eugen_stan.vcf>>
signature.asc
Description: OpenPGP digital signature
