Repository navigation
fix(android): compiling examples, shaded HiveMQ with no packaging excludes, shared-topic unsubscribe - #1960
Merged
Conversation
Object and array examples were emitted without escaping, so a quote inside a value ended the string (the GraphQL mutation examples) and Kotlin interpolated "$id". Kotlin examples also named arguments with the spec's snake_case names (grant_id) instead of the camelCase names the service declares.
🔵 Tier A · Mergeable after minor fixes
The change centralizes Kotlin/Java example-string escaping and camel-cases Kotlin named arguments, with generation coverage for both Kotlin and Android. It also switches Android, Flutter, and React Native to shaded HiveMQ dependencies, removes the packaging-exclude setup and Expo plugin, and guards Android push callbacks after a shared-topic subscription is removed.
Fix with agent prompt### Issue 1
src/SDK/Language/Kotlin.php:169
**Escape carriage returns in string literals too**
An example containing `\r\n` still emits a raw carriage return, which is a line terminator in Java and breaks the generated quoted literal. Please escape `\r` alongside `\n` so Windows-style multiline examples compile too.
```suggestion
$escaped = \str_replace(['\\', '"', "\n", "\r"], ['\\\\', '\\"', '\\n', '\\r'], $value);
```
### Issue 2
templates/react-native/package.json.twig:14
**Preserve compatibility with existing Expo plugin registrations**
Apps following the previous README still have the SDK in `expo.plugins`; after this removal, Expo prebuild cannot resolve its config plugin and fails even though packaging excludes are no longer needed. Keeping a no-op plugin, or documenting that users must remove the existing registration when upgrading, would avoid leaving those integrations broken.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 11
Reviewed |
…excludes The plain client pulls Netty in as separate jars, each with its own META-INF/INDEX.LIST and io.netty.versions.properties, so every app's mergeJavaResource failed until it excluded them, whether or not it used Push. The shaded artifact relocates Netty and JCTools into one jar. Move the consumer R8 rules to the relocated packages and drop the excludes from the Android and Flutter READMEs, the Android example app and the React Native config plugin, whose only job was adding them.
HiveMQ keeps a subscribe callback on its filter until the filter is unsubscribed, and the SDK keeps the filter while another subscription uses it. A removed subscription's callback therefore kept firing. Look the subscription up by id on delivery instead of capturing it.
ChiragAgg5k
force-pushed
the
fix/android-example-literals
branch
from
October 3, 2026 07:53
cfa04b5 to
553a6fc
Compare
ChiragAgg5k
changed the title
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
| */ | ||
| protected function getStringLiteral(string $value, string $lang): string | ||
| { | ||
| $escaped = \str_replace(['\\', '"', "\n"], ['\\\\', '\\"', '\\n'], $value); |
There was a problem hiding this comment.
Escape carriage returns in string literals too
An example containing \r\n still emits a raw carriage return, which is a line terminator in Java and breaks the generated quoted literal. Please escape \r alongside \n so Windows-style multiline examples compile too.
Suggested change
| $escaped = \str_replace(['\\', '"', "\n"], ['\\\\', '\\"', '\\n'], $value); | |
| $escaped = \str_replace(['\\', '"', "\n", "\r"], ['\\\\', '\\"', '\\n', '\\r'], $value); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/SDK/Language/Kotlin.php
Line: 169
Comment:
**Escape carriage returns in string literals too**
An example containing `\r\n` still emits a raw carriage return, which is a line terminator in Java and breaks the generated quoted literal. Please escape `\r` alongside `\n` so Windows-style multiline examples compile too.
```suggestion
$escaped = \str_replace(['\\', '"', "\n", "\r"], ['\\\\', '\\"', '\\n', '\\r'], $value);
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · bug · Reply if this doesn't apply.
| "types": "./types/index.d.ts" | ||
| }, | ||
| "./app.plugin.js": "./app.plugin.js", | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
Preserve compatibility with existing Expo plugin registrations
Apps following the previous README still have the SDK in expo.plugins; after this removal, Expo prebuild cannot resolve its config plugin and fails even though packaging excludes are no longer needed. Keeping a no-op plugin, or documenting that users must remove the existing registration when upgrading, would avoid leaving those integrations broken.
Prompt To Fix With AI
This is a comment left during a code review.
Path: templates/react-native/package.json.twig
Line: 14
Comment:
**Preserve compatibility with existing Expo plugin registrations**
Apps following the previous README still have the SDK in `expo.plugins`; after this removal, Expo prebuild cannot resolve its config plugin and fails even though packaging excludes are no longer needed. Keeping a no-op plugin, or documenting that users must remove the existing registration when upgrading, would avoid leaving those integrations broken.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · bug · Reply if this doesn't apply.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the review findings on appwrite/sdk-for-android#137. All of them come from the generator.
Examples that don't compile (Kotlin and Java)
"mutation { accountUpdateName(name: "Walter") ... }"in both GraphQL mutation examples), and Kotlin interpolated"$id"in the DocumentsDB examples.Kotlin.phpnow builds every example string through one literal helper. It escapes\,"and newlines, plus$in Kotlin; Java leaves$alone.grant_id = ...in every new OAuth2 example). The service methods declare the camelCase names, so the Android and Kotlin example templates now applycaseCamelthe same way the service templates do.No string example in the 2.3.x client spec contains
$,"or\, so plain string examples render the same as before. Only object and array examples change.testExamplesUseKotlinAndJavaLiteralscovers both, for thekotlinandandroidtargets, followingtestObjectArrayExamplesUseSwiftLiterals. The fixture gainsgeneralSnakeCaseParamswith agrant_idquery parameter. It fails onmainand passes here. Rendered against the real 2.3.x client spec, the three flagged files now readgrantId = "<GRANT_ID>",name: \"Walter\"and"\$id" to "example1".Shaded HiveMQ: no packaging excludes for apps
The plain
hivemq-mqtt-clientpulls Netty in as about six separate jars, each with its ownMETA-INF/INDEX.LISTandMETA-INF/io.netty.versions.properties. Android's packager refuses duplicate resources, and only the app can exclude them, so every app had to add excludes, whether or not it used Push. I built the generated example app without them and it failed at:example:mergeDebugJavaResourcewith6 files found with path 'META-INF/INDEX.LIST'.Android, Flutter and React Native now depend on
com.hivemq:hivemq-mqtt-client-shaded:1.3.6, which relocates Netty and JCTools undercom.hivemq.client.internal.shadedinto one jar.push/consumer-rules.pronow names the relocated packages. Run against the shaded jar, the old rules fail R8 with 52 missing classes; the new rules pass.Push: an unsubscribed callback kept firing on a shared topic
HiveMQ keeps a subscribe callback attached to its filter until the filter itself is unsubscribed. The SDK only sends that UNSUBSCRIBE once no other subscription uses the filter. So with two in-process subscriptions on the same topic, unsubscribing one left its callback receiving messages until the other went away or
close()ran. Callbacks now look their subscription up by id when a message arrives, so a removed subscription's callback does nothing. That also covers a message already in flight whenunsubscribe()returns.The Android e2e adds
Push shared topic unsubscribe: it subscribes twice toe2e-push, unsubscribes one, triggers another publish, and asserts only the remaining subscription receives it.Android5Java17without the fix fails on exactly that line (223 of 224 assertions pass); with the fix it passes.Related: appwrite-labs/cloud#6299