Skip to content

Add @Required annotation with optional validator#1558

Merged
WilliamBergamin merged 12 commits intoslackapi:mainfrom
fst-john:fstj_add_required_annotation
Feb 26, 2026
Merged

Add @Required annotation with optional validator#1558
WilliamBergamin merged 12 commits intoslackapi:mainfrom
fst-john:fstj_add_required_annotation

Conversation

@fst-john
Copy link
Contributor

@fst-john fst-john commented Feb 24, 2026

Adds a new @Required annotation to the model package for use in marking attributes in model classes as "required". This is so we can annotate attributes of our model classes that are expected to be present on every instance of the object, so developers can safely access without any explicit null checks. Meant to be a drop-in replacement for in-line code comments or javadocs that state the same (but apply no actual enforcement at runtime)

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

@fst-john fst-john requested a review from a team as a code owner February 24, 2026 20:34
@vegeris
Copy link
Contributor

vegeris commented Feb 24, 2026

Looks like this has been a requested feature for GSON for some time now

@fst-john
Copy link
Contributor Author

Looks like this has been a requested feature for GSON for some time now

Oh yeah nice! Really the only thing I added in this implementation was to allow folks to define their own predicate that should be applied against the field that the annotation was added on. This was mainly so that folks could handle primitive types (if they wanted) since these get boxed automatically by the JVM. We could make it "dumber" though and do something similar to the stack overflow link where primitives are checked against their JVM defaults, that kinda thing 🤷‍♂️

@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

❌ Patch coverage is 70.58824% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.35%. Comparing base (797d1c1) to head (e8ca214).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../json/RequiredPropertyDetectionAdapterFactory.java 76.92% 7 Missing and 2 partials ⚠️
...main/java/com/slack/api/util/json/GsonFactory.java 37.50% 3 Missing and 2 partials ⚠️
...lient/src/main/java/com/slack/api/SlackConfig.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1558      +/-   ##
============================================
+ Coverage     73.28%   73.35%   +0.06%     
- Complexity     4513     4527      +14     
============================================
  Files           477      479       +2     
  Lines         14274    14321      +47     
  Branches       1487     1495       +8     
============================================
+ Hits          10461    10505      +44     
- Misses         2922     2927       +5     
+ Partials        891      889       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Nice work!! 💯 I think the approach is good 🚀

I left mainly minor comments regarding adhering to existing patterns in the project, creating a new directory for the annotations and improving test coverage

Thanks for putting this together

@@ -0,0 +1,5 @@
package com.slack.api.model.annotation;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking we move this to a new package/directory

/**
 * Provides JSON custom annotation utilities for the classes in this library.
 */
package com.slack.api.util.annotations;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that these predicates are meant to live on slack model objects I think it makes sense for it to live in the model package, but I'm open to a new sub-module under model; maybe predicates (or something?)

package com.slack.api.model.annotation;

public interface FieldPredicate {
boolean test(Object obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job on the interface approach 🚀 but I think the test terminology might be confusing, what do you think of check or validate instead?

Maybe validate is better since it aligns with validator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's fair; I went with test because that's the abstract method on the Predicate functional interface https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html

registerTypeAdapters(builder, failOnUnknownProps);
}

public static void registerTypeAdapters(GsonBuilder builder, boolean failOnUnknownProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why this is public but I don't wanna break anybody, so leaving it as is 🤷‍♂️

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Looking good 💯 thanks for addressing my comments

Left 2 more comments! let mw know what you think

The one on the test GsonFactory is a nice to have and aims to maintain existing pattern in the project

registerTypeAdapters(gsonBuilder, failOnUnknownProps);

if (failOnUnknownProps || config.isLibraryMaintainerMode()) {
gsonBuilder = gsonBuilder.registerTypeAdapterFactory(new UnknownPropertyDetectionAdapterFactory());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reassignment is redundant, so removing it

gsonBuilder.registerTypeAdapterFactory(new RequiredPropertyDetectionAdapterFactory());
}
if (config.isPrettyResponseLoggingEnabled()) {
gsonBuilder = gsonBuilder.setPrettyPrinting();
Copy link
Contributor Author

@fst-john fst-john Feb 26, 2026

Choose a reason for hiding this comment

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

Same thing here - there's no need for the re-assignmen

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Nice thanks for working on this 🚀 And your patients with the feedback 🙏

@WilliamBergamin WilliamBergamin enabled auto-merge (squash) February 26, 2026 21:46
@WilliamBergamin WilliamBergamin merged commit 372fff8 into slackapi:main Feb 26, 2026
5 of 12 checks passed
@fst-john fst-john deleted the fstj_add_required_annotation branch February 26, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants