Skip to content

fix: warn when Kotlin class loses field values due to missing kotlin-reflect#7675

Merged
wenshao merged 2 commits into
alibaba:mainfrom
e5l:kotlin-reflect-warning
Jul 15, 2026
Merged

fix: warn when Kotlin class loses field values due to missing kotlin-reflect#7675
wenshao merged 2 commits into
alibaba:mainfrom
e5l:kotlin-reflect-warning

Conversation

@e5l

@e5l e5l commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What / Why

When a Kotlin class with constructor parameters (e.g. any data class) is
deserialized and kotlin-reflect is not on the classpath, FastJSON2 cannot
resolve the constructor parameter names. The result is a silent failure: an
object is constructed, but its fields are left at their default/null values with
no error and no diagnostic. Users only discover the data loss downstream, and it
looks like a FastJSON2 bug rather than a missing dependency.

This is the worst kind of failure mode — wrong data, no signal.

Root cause

KotlinUtils.getConstructor(...) only resolves parameter names when
STATE == 2, i.e. when kotlin.reflect.jvm.ReflectJvmMapping (shipped in
kotlin-reflect) is present:

if (creatorParams != 0 && STATE == 2) {
    // ... resolve names via Reflection.getOrCreateKotlinClass(...)
}

When kotlin-reflect is absent, STATE == 1 (only kotlin-stdlib is present),
the block is skipped, beanInfo.createParameterNames stays null, and binding
falls back to defaults with no indication of why.

Change

Adds a diagnostic warning when — and only when — this exact condition is hit:
the class is Kotlin (the call site in ObjectReaderBaseModule is already guarded
by beanInfo.kotlin), it has constructor parameters, kotlin-reflect is
unavailable, and parameter names were not resolved by any other means
(annotations / @JSONCreator):

} else if (creatorParams != 0 && beanInfo.createParameterNames == null) {
    System.err.println("fastjson2 warning: class " + clazz.getName()
        + " is a Kotlin class with constructor parameters, "
        + "but kotlin-reflect is not on the classpath. ...");
}

The message names the class and tells the user to add
org.jetbrains.kotlin:kotlin-reflect.

No behavior change beyond the warning; the fallback path is unchanged. Java
classes are unaffected (guarded by beanInfo.kotlin), and users who already
supply parameter names are not warned (guarded by createParameterNames == null).

…reflect

When a Kotlin data class has constructor parameters but kotlin-reflect is
not on the classpath, parameter names cannot be resolved. This causes
deserialization to silently produce objects with default/null field values.

Add a diagnostic warning to stderr when this condition is detected, so
users get a clear pointer to the root cause instead of debugging silent
data loss.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Jul 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Member

Suggestions — commit ee938bbaca4449d2f4512c75ccbb0c95a025044a

File Issue Suggested fix
KotlinUtils.java:105-111 No deduplication guard — warning fires once per distinct Kotlin class. An app with N Kotlin data classes emits N separate System.err.println lines at startup. Add a static volatile Set<Class<?>> or volatile boolean guard to warn at most once per class (or once globally).
KotlinUtils.java:107 Misleading message for serialization pathgetConstructor is called from both ObjectReaderBaseModule (deserialization) and ObjectWriterBaseModule (serialization), but the message says "will cause deserialization to silently lose field values". Soften to "may cause deserialization issues" or only emit the warning from the reader path.
KotlinUtils.java:83-103 kotlin-reflect runtime failure goes unwarned — when STATE==2 but kotlin-reflect fails at runtime (e.g., JDK 17+ module restrictions), catch (Throwable) silently swallows the error. The else if is never reached since the if was already true. After the catch block, check if createParameterNames is still null and emit a warning indicating kotlin-reflect was found but failed.
KotlinUtils.java:105 No test coverage — the new else if branch is never exercised by existing tests (all Kotlin tests run with kotlin-reflect present, STATE=2). Add a test that simulates STATE < 2 or mocks a BeanInfo with null createParameterNames.
KotlinUtils.java:105 System.err.println inconsistent with codebase — this is the only System.err.println in all of core/src/main/java/. It bypasses logging frameworks and cannot be suppressed. Use java.util.logging.Logger at WARNING level (JDK-standard, zero extra dependency).

— qwen3.7-max via Qwen Code /review

- replace System.err.println with java.util.logging at WARNING level
- deduplicate: warn at most once per class
- warn only from the deserialization call site (ObjectReaderBaseModule);
  the writer path does not need constructor parameter names
- also warn when kotlin-reflect is present but fails at runtime
  (previously swallowed by catch (Throwable))
- add unit tests for the warn conditions and an integration test
  asserting no false-positive warning when kotlin-reflect is available

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@e5l
e5l requested a review from wenshao July 9, 2026 12:17

@wenshao wenshao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅

Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

@e5l
e5l requested a review from wenshao July 13, 2026 06:39
@wenshao
wenshao merged commit f68fd88 into alibaba:main Jul 15, 2026
30 checks passed
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.

3 participants