Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GeneralCodingRule to discourage use of field injection #289

Merged
merged 5 commits into from
Apr 6, 2020

Conversation

rweisleder
Copy link
Contributor

Resolves #288

@ghost
Copy link

ghost commented Dec 31, 2019

Congratulations 🎉. DeepCode analyzed your code in 1.293 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the initiative! I guess this rule does make sense, since it's coming up again and again 😉
I wrote some comments and would be interested in your take on it!

rweisleder and others added 5 commits April 6, 2020 15:18
This way we can easier reuse centrally managed dependencies in other contexts like the `maven-integration-test`.

Signed-off-by: Peter Gafert <[email protected]>
We should reuse the centrally managed dependencies for both the example and the `maven-integration-test`. To achieve this I have extended the simple pom templating mechanism to support replacements like `#{dependency.junit4:test}` by the respective Maven dependency `<dependency>...</dependency>`.

Signed-off-by: Peter Gafert <[email protected]>
because we want to have one example for every library rule, but then the unit test is obsolete. Also this way it is consistent to how the other rules are tested and we keep the core `archunit` module free of extra dependencies for frameworks.
Note that the Shadow plugin actually rewrites all string occurrences of a package to repackage within the bytecode. Thus I had to narrow the relocation of `com.google` for Guava to `com.google.common` and `com.google.thirdparty`, because otherwise the string constant `com.google.inject` is actually rewritten in the bytecode to `com.tngtech.archunit.thirdparty.com.google.inject` and the rule does not work anymore.

Signed-off-by: Peter Gafert <[email protected]>
Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally got around to looking into it again, thanks a lot!! 😃
I've squashed your commits, hope that is okay. Then I added a review commit migrating the test to an example + integration test, because that is how all the other general rules are tested (that way there is also an example in the example project for people to check out).
When I did this and added a Google Guice test, I noticed something really unexpected though 🤦‍♂️ . The Gradle Shadow plugin rewrites all string constants in the bytecode. So even though you wrote stringly "com.google.inject.Inject" without any reference to a class, in the bytecode this constant became com.tngtech.archunit.thirdparty.com.google.inject.Inject rendering the condition useless. I've only noticed because I migrated it to an example rule and thus the Maven Integration test failed (since that one actually uses the real packaged archives).
Anyway, now I've also improved the Maven Integration test a little to build the POM from the managed dependencies, so that's a nice side effect 😃
That's why I love a sufficient number of black-box tests against the final release though...

@codecholeric codecholeric merged commit bc7ddf7 into TNG:master Apr 6, 2020
@rweisleder rweisleder deleted the gh-288 branch April 8, 2020 16:42
codecholeric added a commit that referenced this pull request Feb 21, 2021
Disallow fields to be annotated with common injection annotations like `@Autowired` or `@Inject` to encourage constructor injection instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default condition to avoid field Injection and haveConstructor Injection
4 participants