Skip to content

Commit 11e22a9

Browse files
Copilotandimarek
Copilot
and
andimarek
committed
Add comprehensive JSpecify migration plan document
Co-authored-by: andimarek <1706744+andimarek@users.noreply.github.com>
1 parent 5bc117e commit 11e22a9

File tree

1 file changed

+256
-0
lines changed

1 file changed

+256
-0
lines changed

‎JSPECIFY_MIGRATION_PLAN.md‎

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
# JSpecify Migration Plan for java-dataloader
2+
3+
This document outlines the plan for adding comprehensive [JSpecify](https://jspecify.dev/) nullness annotations to the java-dataloader library.
4+
5+
## Current State
6+
7+
The repository already has partial JSpecify integration:
8+
9+
- **JSpecify 1.0.0 dependency** is configured as an API dependency
10+
- **NullAway** is configured with JSpecify mode (`JSpecifyMode=true`) and `OnlyNullMarked=true`
11+
- **~21 files** are already annotated with `@NullMarked` and `@Nullable`
12+
- OSGi bundle configuration makes JSpecify annotations optional for users
13+
14+
### Already Annotated Files
15+
16+
The following files already have `@NullMarked` annotations:
17+
18+
#### Core Package (`org.dataloader`)
19+
- `DataLoader.java` ✅
20+
- `DataLoaderFactory.java` ✅ (assumed based on usage)
21+
- `DataLoaderHelper.java` ✅
22+
- `DataLoaderRegistry.java` ✅
23+
- `BatchLoader.java` ✅
24+
- `BatchLoaderWithContext.java` ✅
25+
- `BatchLoaderContextProvider.java` ✅
26+
- `BatchLoaderEnvironment.java` ✅
27+
- `BatchLoaderEnvironmentProvider.java` ✅
28+
- `MappedBatchLoader.java` ✅
29+
- `MappedBatchLoaderWithContext.java` ✅
30+
- `BatchPublisher.java` ✅
31+
- `BatchPublisherWithContext.java` ✅
32+
- `MappedBatchPublisher.java` ✅
33+
- `MappedBatchPublisherWithContext.java` ✅
34+
- `CacheMap.java` ✅
35+
- `CacheKey.java` ✅
36+
- `ValueCache.java` ✅
37+
- `ValueCacheOptions.java` ✅
38+
- `DelegatingDataLoader.java` ✅
39+
- `DispatchResult.java` ✅
40+
41+
#### Implementation Package (`org.dataloader.impl`)
42+
- `DefaultCacheMap.java` ✅
43+
44+
#### Registries Package (`org.dataloader.registries`)
45+
- `ScheduledDataLoaderRegistry.java` ✅
46+
47+
## Files Requiring Annotations
48+
49+
### Phase 1: Core Package (`org.dataloader`) - High Priority
50+
51+
| File | Status | Notes |
52+
|------|--------|-------|
53+
| `Try.java` | ❌ Needs annotation | Generic type `V` may be nullable; internal throwable field is nullable |
54+
| `DataLoaderOptions.java` | ❌ Needs annotation | Many nullable fields (cacheKeyFunction, cacheMap, valueCache, batchLoaderScheduler) |
55+
56+
### Phase 2: Implementation Package (`org.dataloader.impl`)
57+
58+
| File | Status | Notes |
59+
|------|--------|-------|
60+
| `Assertions.java` | ❌ Needs annotation | `nonNull()` method accepts nullable input, returns non-null |
61+
| `CompletableFutureKit.java` | ❌ Needs annotation | Review for nullable handling |
62+
| `PromisedValues.java` | ❌ Needs annotation | Interface with nullable considerations |
63+
| `PromisedValuesImpl.java` | ❌ Needs annotation | Implementation class |
64+
| `NoOpValueCache.java` | ❌ Needs annotation | Implements `ValueCache` |
65+
| `DataLoaderAssertionException.java` | ❌ Needs annotation | Exception class |
66+
67+
### Phase 3: Statistics Package (`org.dataloader.stats`)
68+
69+
| File | Status | Notes |
70+
|------|--------|-------|
71+
| `Statistics.java` | ❌ Needs annotation | Simple data class, likely all non-null |
72+
| `StatisticsCollector.java` | ❌ Needs annotation | Interface |
73+
| `SimpleStatisticsCollector.java` | ❌ Needs annotation | Implements `StatisticsCollector` |
74+
| `ThreadLocalStatisticsCollector.java` | ❌ Needs annotation | Implements `StatisticsCollector` |
75+
| `NoOpStatisticsCollector.java` | ❌ Needs annotation | Implements `StatisticsCollector` |
76+
| `DelegatingStatisticsCollector.java` | ❌ Needs annotation | Implements `StatisticsCollector` |
77+
78+
#### Statistics Context Subpackage (`org.dataloader.stats.context`)
79+
80+
| File | Status | Notes |
81+
|------|--------|-------|
82+
| `IncrementBatchLoadCountByStatisticsContext.java` | ❌ Needs annotation | Context class |
83+
| `IncrementBatchLoadExceptionCountStatisticsContext.java` | ❌ Needs annotation | Context class |
84+
| `IncrementCacheHitCountStatisticsContext.java` | ❌ Needs annotation | Context class |
85+
| `IncrementLoadCountStatisticsContext.java` | ❌ Needs annotation | Context class |
86+
| `IncrementLoadErrorCountStatisticsContext.java` | ❌ Needs annotation | Context class |
87+
88+
### Phase 4: Instrumentation Package (`org.dataloader.instrumentation`)
89+
90+
| File | Status | Notes |
91+
|------|--------|-------|
92+
| `DataLoaderInstrumentation.java` | ❌ Needs annotation | Methods return `@Nullable DataLoaderInstrumentationContext` |
93+
| `DataLoaderInstrumentationContext.java` | ❌ Needs annotation | Interface with nullable generics |
94+
| `DataLoaderInstrumentationHelper.java` | ❌ Needs annotation | Helper with NOOP implementations |
95+
| `SimpleDataLoaderInstrumentationContext.java` | ❌ Needs annotation | Simple implementation |
96+
| `ChainedDataLoaderInstrumentation.java` | ❌ Needs annotation | Chains multiple instrumentations |
97+
98+
### Phase 5: Scheduler Package (`org.dataloader.scheduler`)
99+
100+
| File | Status | Notes |
101+
|------|--------|-------|
102+
| `BatchLoaderScheduler.java` | ❌ Needs annotation | Interface; `environment` parameter documented as possibly null |
103+
104+
### Phase 6: Registries Package (`org.dataloader.registries`)
105+
106+
| File | Status | Notes |
107+
|------|--------|-------|
108+
| `DispatchPredicate.java` | ❌ Needs annotation | Functional interface |
109+
110+
### Phase 7: Reactive Package (`org.dataloader.reactive`)
111+
112+
| File | Status | Notes |
113+
|------|--------|-------|
114+
| `ReactiveSupport.java` | ❌ Needs annotation | Factory methods for subscribers |
115+
| `AbstractBatchSubscriber.java` | ❌ Needs annotation | Base subscriber implementation |
116+
| `BatchSubscriberImpl.java` | ❌ Needs annotation | Subscriber implementation |
117+
| `MappedBatchSubscriberImpl.java` | ❌ Needs annotation | Subscriber implementation |
118+
119+
### Phase 8: Annotations Package (`org.dataloader.annotations`)
120+
121+
| File | Status | Notes |
122+
|------|--------|-------|
123+
| `ExperimentalApi.java` | ❌ Consider | Annotation definition |
124+
| `GuardedBy.java` | ❌ Consider | Annotation definition |
125+
| `Internal.java` | ❌ Consider | Annotation definition |
126+
| `PublicApi.java` | ❌ Consider | Annotation definition |
127+
| `PublicSpi.java` | ❌ Consider | Annotation definition |
128+
| `VisibleForTesting.java` | ❌ Consider | Annotation definition |
129+
130+
> **Note**: Annotation definitions typically don't need `@NullMarked` unless they have methods that return potentially null values.
131+
132+
## Implementation Strategy
133+
134+
### Step 1: Add `@NullMarked` to Each Class
135+
136+
For each file, add the `@NullMarked` annotation at the class level:
137+
138+
```java
139+
import org.jspecify.annotations.NullMarked;
140+
141+
@NullMarked
142+
public class MyClass {
143+
// ...
144+
}
145+
```
146+
147+
### Step 2: Mark Nullable Elements
148+
149+
For any parameter, return type, or field that can be null, add `@Nullable`:
150+
151+
```java
152+
import org.jspecify.annotations.Nullable;
153+
154+
@NullMarked
155+
public class Example {
156+
private @Nullable String optionalField;
157+
158+
public @Nullable Result findResult(String key) {
159+
// ...
160+
}
161+
162+
public void process(@Nullable String maybeNull) {
163+
// ...
164+
}
165+
}
166+
```
167+
168+
### Step 3: Handle Nullable Generic Type Parameters
169+
170+
For generic types where the type parameter can be null:
171+
172+
```java
173+
// Type parameter V can be null
174+
public class Try<V extends @Nullable Object> {
175+
private @Nullable V value;
176+
}
177+
178+
// In DataLoader, V can be null
179+
public class DataLoader<K, V extends @Nullable Object> {
180+
// ...
181+
}
182+
```
183+
184+
### Step 4: Verify with NullAway
185+
186+
After annotating each phase, run:
187+
188+
```bash
189+
./gradlew compileJava
190+
```
191+
192+
NullAway will report any nullability errors. Fix any issues before proceeding.
193+
194+
### Step 5: Enable Full Package Checking (Final Step)
195+
196+
Once all files are annotated, uncomment the following in `build.gradle`:
197+
198+
```gradle
199+
option("NullAway:AnnotatedPackages", "org.dataloader")
200+
```
201+
202+
This will enable strict null checking for the entire package.
203+
204+
## Testing Strategy
205+
206+
1. **Compile-time verification**: NullAway catches nullability issues during compilation
207+
2. **Existing tests**: Run the full test suite to ensure no behavioral regressions
208+
3. **IDE integration**: JSpecify annotations work with IDEs that support them (IntelliJ, Eclipse with plugins)
209+
210+
## Compatibility Considerations
211+
212+
### Binary Compatibility
213+
- Adding JSpecify annotations is binary compatible
214+
- The annotations are available at runtime but not required (resolution:=optional in OSGi)
215+
216+
### Source Compatibility
217+
- Users who don't use null-checking tools will see no difference
218+
- Users with NullAway/Checker Framework/IntelliJ will benefit from null checking
219+
220+
### API Design Decisions
221+
222+
When annotating the API, consider:
223+
224+
1. **Return types**: Should methods return `@Nullable` or use `Optional`?
225+
- Existing uses of `Optional` remain unchanged
226+
- Raw nullable returns get `@Nullable`
227+
228+
2. **Parameters**: Document and annotate nullable parameters
229+
- If the Javadoc says "can be null", add `@Nullable`
230+
231+
3. **Generic bounds**: For generics that can hold null values
232+
- Use `V extends @Nullable Object` pattern
233+
234+
## Priority Order
235+
236+
1. **Core public API** (`org.dataloader` package) - Users interact with these directly
237+
2. **SPI interfaces** (`instrumentation`, `scheduler`) - Extension points
238+
3. **Statistics** - Commonly used for monitoring
239+
4. **Implementation details** (`impl`, `reactive`) - Internal but important for correctness
240+
241+
## Estimated Effort
242+
243+
- **Phase 1-2** (Core + Impl): ~8 files, 2-3 hours
244+
- **Phase 3** (Stats): ~11 files, 1-2 hours
245+
- **Phase 4-5** (Instrumentation + Scheduler): ~6 files, 1-2 hours
246+
- **Phase 6-7** (Registries + Reactive): ~5 files, 1 hour
247+
- **Phase 8** (Annotations): ~6 files, 30 minutes
248+
- **Final verification and testing**: 1-2 hours
249+
250+
**Total estimated effort**: 6-10 hours
251+
252+
## References
253+
254+
- [JSpecify User Guide](https://jspecify.dev/docs/user-guide/)
255+
- [NullAway Wiki](https://github.com/uber/NullAway/wiki)
256+
- [JSpecify 1.0.0 Release Notes](https://jspecify.dev/blog/jspecify-1.0/)

0 commit comments

Comments
 (0)