Skip to content

Detect and prohibit new Guava classes #3869

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

Merged
merged 3 commits into from
Apr 3, 2025
Merged

Detect and prohibit new Guava classes #3869

merged 3 commits into from
Apr 3, 2025

Conversation

dondonz
Copy link
Member

@dondonz dondonz commented Mar 29, 2025

Fixes #3859

Problem

We want to keep dependencies to an absolute minimum as this is a low level library. However we make an exception for a few selected Guava classes which are shaded in.

There's a potential risk where someone uses a new Guava class in a pull request, and either:

  1. They are unaware that they should, where possible, use a non-Guava version of the method (which is different to their usual developer workflow)
  2. Or they really must use a Guava class, we forget to shade in the new class in the Gradle build file, causing functionality to break

Solution

This PR adds an Archunit test to detect new Guava class usage and produce a helpful error message to guide contributors to 1. use non-Guava versions where possible or 2. adjust the Gradle build file to shade in the new class

Sample output

Example of deliberately failing pipeline (before I added the full set of exemptions): https://github.com/graphql-java/graphql-java/pull/3869/checks?check_run_id=39620708772

GuavaLimitCheck > should identify which classes use prohibited Guava dependencies STANDARD_OUT
    Prohibited Guava class usage found:

    Class: graphql.execution.ExecutionStrategy uses these prohibited Guava classes:
      - JavaClass{name='com.google.common.collect.Maps'}

    Class: graphql.schema.diffing.DiffImpl uses these prohibited Guava classes:
      - JavaClass{name='com.google.common.collect.Multiset'}
      - JavaClass{name='com.google.common.collect.HashMultiset'}
      - JavaClass{name='com.google.common.collect.Multisets'}

    Either:
    1. Preferred option: Replace them with alternatives that don't depend on Guava
    2. If absolutely necessary: Add these Guava classes to the shade configuration in the build.gradle file

Note

We don't yet use anything from the math or primitives packages directly, but these are transitively used by collect, so they also need to be shaded in

 relocate('com.google.common', 'graphql.com.google.common') {
        include 'com.google.common.collect.*'
        include 'com.google.common.base.*'
        include 'com.google.common.math.*'
        include 'com.google.common.primitives.*'
    }

Copy link
Contributor

github-actions bot commented Mar 29, 2025

Test Results

  312 files    312 suites   48s ⏱️
3 577 tests 3 571 ✅ 6 💤 0 ❌
3 666 runs  3 660 ✅ 6 💤 0 ❌

Results for commit 6fb0db2.

♻️ This comment has been updated with latest results.

@dondonz dondonz added this to the 23.0 breaking changes milestone Mar 29, 2025
@dondonz dondonz added the not release related changes which are not released (for example unit tests or docs) label Mar 29, 2025
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Nice one

@dondonz dondonz merged commit 9099f7a into master Apr 3, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not release related changes which are not released (for example unit tests or docs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore how to prohibit using certain new Guava classes
2 participants