bbottema / email-rfc2822-validator

The world's only Java-based rfc2822-compliant email address validator and parser
64 stars 13 forks source link

Consider making jakarata.mail as optional and make slf4j as scope test #21

Closed agentgt closed 3 years ago

agentgt commented 3 years ago

The email validation doesn't use or need Jakarta.mail.

I know there is a convenient parser but I would either move that parser to a separate maven module or just make jakarta.mail as <optional>true</optional>.

Also slf4j is not used anywhere that I could tell in src/main/java.

I assume it was or is used in src/test/java. Regardless I wouldn't expect a validation library to use or need logging.

bbottema commented 3 years ago

It does actually use it, specifically the class javax.mail.internet.InternetAddress. I've tried to replace it with a POJO in the past, but it turns out we use quite a lot of functionality provided by InternetAddress so I abandoned the approach at the time.

agentgt commented 3 years ago

Validation aka EmailAddressValidator.isValid(email). Does not use or need javax.mail.internet.InternetAddress

Try it yourself:

    <dependency>
        <groupId>com.github.bbottema</groupId>
        <artifactId>emailaddress-rfc2822</artifactId>
        <exclusions>
          <exclusion>
            <groupId>com.sun.activation</groupId>
            <artifactId>jakarta.activation</artifactId>
          </exclusion>
          <exclusion>
            <groupId>com.sun.mail</groupId>
            <artifactId>jakarta.mail</artifactId>
          </exclusion>
        </exclusions>
    </dependency>

The only class that uses it is EmailAddressParser and per EmailAddressParser javadoc

 * A utility class to parse, clean up, and extract email addresses from messages per RFC2822 syntax. Designed to integrate with Javamail (this class will
 * require that you have a javamail mail.jar in your classpath), but you could easily change the existing methods around to not use Javamail at all

So arguably it isn't required and I think the parsing should be moved to a separate module (if you need help with multi-modules maven I can put in a PR). I'm not faulting the project for using javax.mail.internet.InternetAddress as I have used it myself as well Apache Mime4J as its the only mime parser I know that works with vast amount of inputs. However most folks I assume are just looking for validation and not parsing of email addresses (nor mime).

Secondly you didn't address the SLF4J issue. I can't see a place where you used it.

bbottema commented 3 years ago

Where do you see slf4j? I can't find any references to it.?

/edit: Ahh, it's in the standard project parent POM, and for compile dependency only the API is included. Are you facing issues? Excluding a dependency obtained through a parent POM is not a trivial task.

bbottema commented 3 years ago

I've released 2.3.0, which made Jakarta Mail an optional dependency.

agentgt commented 3 years ago

Ahh, it's in the standard project parent POM, and for compile dependency only the API is included. Are you facing issues? Excluding a dependency obtained through a parent POM is not a trivial task.

No I didn't realize it wasn't a trivial task. I have no problem excluding the jars.

I can understand why you closed the issue original and I'm just jaded with a lot of projects not really managing their dependencies well and or just requiring a dependency for a single class. This project is actually pretty high quality and isn't that bad.

Dependency management is a huge technical debt overtime. At least for us it is. I can't just will nilly add a dependency to our code base especially if it adds dependencies. I have to check it out.

Let me give you an example for your project (and btw I totally appreciate all the time you have put into it).

Because this project uses javax.mail aka jakarta.mail it pulls in the activation library implementation aka com.sun.activation:jakarta.activation. Meanwhile other projects like jOOQ for example that use JAXB pull the older javax.activation-api. OK so we need to exclude and replace javax.activation-api with jakarta.activation:jakarta.activation-api right? Wrong. You see com.sun.activation:jakarta.activation has overlapping classes with jakarta.activation:jakarta.activation-api so you have to exclude the API when you actually use com.sun.activation:jakarta.activation. This was an hour so. All of the above for a one POJO.

I admit the above is confusing and whiny. Such is the life of dealing with Java dependencies. The irony is I picked your library because I thought it didn't have any such dependency problems. In the past when I have been OK with javax.mail aka jakarta.mail InternetAddress[] ia = InternetAddress.parse(address, true) has been good enough.

Regardless this project is very valuable and worse case scenario I can fork it. Thank you for putting it together.

I've released 2.3.0, which made Jakarta Mail an optional dependency.

Thank you!