Fix null EventProperties.getType() on Android (initialize eventType)#1483
Fix null EventProperties.getType() on Android (initialize eventType)#1483bmehta001 wants to merge 3 commits into
Conversation
…ype() EventPropertiesStorage's default constructor initialized every member except eventType, so EventProperties.getType() returned null on a freshly constructed object instead of its documented empty-string default (microsoft#1329). On Android/Java this surfaced as a NullPointerException for callers. - EventPropertiesStorage(): initialize eventType = "" (matching eventName and the other members). - EventsUnitTest: add newEventPropertiesGetTypeReturnsEmptyString as a regression test (getType() on a new EventProperties is non-null and ""). Resolves microsoft#1329. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Validated locally on a plain JVM (JDK 17, WSL) by compiling the actual With the fix: Negative control (remove the So the one-line initialization is precisely what makes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an Android/Java contract violation where EventProperties.getType() could return null for newly constructed objects, despite its Javadoc specifying an empty-string default. It does so by initializing the underlying storage field and adding a regression test to prevent reintroduction.
Changes:
- Initialize
EventPropertiesStorage.eventTypeto""in the default constructor to avoidnullreturns fromgetType(). - Add a JUnit regression test ensuring
EventProperties.getType()is non-null and equals""immediately after construction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/EventPropertiesStorage.java | Initializes eventType to an empty string to match EventProperties.getType()’s documented default behavior. |
| lib/android_build/maesdk/src/test/java/com/microsoft/applications/events/EventsUnitTest.java | Adds a regression test validating getType() returns "" (not null) on a freshly constructed EventProperties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code review of the microsoft#1329 fix found a native-path regression. Making Java getType() return "" (instead of null) fixes the Java-side NPE, but the JNI converter forwarded the type to native whenever the jstring was non-null: if (jstrEventType != NULL) eventProperties.SetType(JStringToStdString(env, jstrEventType)); Before microsoft#1329, getType() returned null for a default EventProperties, so SetType was skipped and native eventType stayed at its "" default. Now the non-null "" reaches native SetType(""), which fails validateEventName (length < 4) and, for EVERY typeless event, logs "Invalid event type!" and broadcasts an EVT_REJECTED DebugEvent to all registered listeners (EventProperties.cpp SetType -> ILManager::DispatchEventBroadcast) -- a false-positive rejection signal indistinguishable from a genuinely rejected event. Fix: in the JNI converter (the sole native SetType chokepoint), treat an empty type as "unset" and only call SetType for a non-empty string, restoring the pre-microsoft#1329 native behavior while keeping the corrected Java API contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // Regression test for #1329: getType() on a freshly constructed | ||
| // EventProperties must return its documented default of "" (an empty | ||
| // string), not null -- EventPropertiesStorage previously left eventType | ||
| // uninitialized. | ||
| EventProperties props = new EventProperties("MyEvent", DiagnosticLevel.DIAG_LEVEL_OPTIONAL); | ||
| assertNotNull(props.getType()); | ||
| assertEquals("", props.getType()); | ||
| } |
…ly (no native) The regression test constructed EventProperties, whose constructor calls setName() -> native Utils.validateEventName(). These are JVM unit tests (@RunWith(MockitoJUnitRunner)) with no native library loaded, so it would throw UnsatisfiedLinkError instead of exercising the regression. Test the pure-Java EventPropertiesStorage (same package) directly -- the exact class the microsoft#1329 fix initialized (eventType = ""). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What
Initialize
EventPropertiesStorage.eventTypeto""soEventProperties.getType()returns its documented empty-string default instead ofnullon a freshly constructed object.Why
Per #1329, on Android/Java:
EventPropertiesStorage()'s default constructor initialized every member (eventName,eventLatency,eventPersistence, …) excepteventType, leaving itnull. ButgetType()'s contract (its Javadoc) states "An empty string is returned if this was never set." So the implementation violated its own contract.Fix
EventPropertiesStorage()— initializeeventType = ""(right aftereventName = "", matching the existing pattern).EventsUnitTest— addnewEventPropertiesGetTypeReturnsEmptyString, assertinggetType()on a newEventPropertiesis non-null and equal to"".The copy constructor already propagates
eventType, and since all default-constructed storages now set"", copies are""too.Validation
The change is a one-line field initialization plus a JUnit regression test using already-imported assertions and the pure-Java
EventProperties(String, DiagnosticLevel)constructor. I couldn't rungradlew maesdk:testlocally (no JDK/Android toolchain on this dev box), so it relies on the Android CI (Build for Androidworkflow runsmaesdk:test) for the build+test confirmation.Resolves #1329.