Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[expo-updates][android][sqlite] make assets.type nullable #14499

Merged
merged 11 commits into from
Sep 28, 2021

Conversation

jkhales
Copy link
Contributor

@jkhales jkhales commented Sep 21, 2021

Why

Make the "type" column in the "assets" table nullable.

Since launch assets don't require a file extension, we are no longer sending one.

We want to make iOS match this behavior in a future PR.

How

Followed this: #12084

Test Plan

made new tests and confirmed on a demo repo that loading a project that has no file extension included with its launch asset, served by expo start, crashes Expo Go and after this PR is applied no longer crashes it.

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@linear
Copy link

linear bot commented Sep 21, 2021

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 21, 2021
@jkhales jkhales force-pushed the jonathan/eng-1928-fileextension-android branch from 3549e03 to cc3c566 Compare September 22, 2021 00:30
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 23, 2021
},
{
"tableName": "assets",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, `url` TEXT, `key` TEXT UNIQUE, `headers` TEXT, `type` TEXT, `metadata` TEXT, `download_time` INTEGER, `relative_path` TEXT, `hash` BLOB, `hash_type` INTEGER NOT NULL, `marked_for_deletion` INTEGER NOT NULL)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the index_assets_key index and replaced it with "key TEXT UNIQUE" to match the iOS schema.

@jkhales jkhales requested a review from esamelson September 23, 2021 17:50
@jkhales jkhales marked this pull request as ready for review September 23, 2021 17:50
@jkhales jkhales force-pushed the jonathan/eng-1928-fileextension-android branch from 46c9139 to e5e22e1 Compare September 23, 2021 17:51
@@ -39,6 +39,8 @@ public static synchronized UpdatesDatabase getInstance(Context context) {
if (sInstance == null) {
sInstance = Room.databaseBuilder(context, UpdatesDatabase.class, DB_NAME)
.addMigrations(MIGRATION_4_5)
.addMigrations(MIGRATION_5_6)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added after discussion with @esamelson that it should have been included.

@jkhales jkhales force-pushed the jonathan/eng-1928-fileextension-android branch from e3cc972 to 99233f3 Compare September 24, 2021 01:44
@jkhales jkhales changed the title Jonathan/eng 1928 fileextension android [expo-updates][android][sqlite] make assets.type nullable Sep 24, 2021
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

This looks good. Have you also checked all usages of AssetEntity.type to ensure they account for the possibility of a null value at runtime?

@jkhales
Copy link
Contributor Author

jkhales commented Sep 28, 2021

This looks good. Have you also checked all usages of AssetEntity.type to ensure they account for the possibility of a null value at runtime?

Yea, the only place it gets read is here: https://github.com/expo/expo/blob/master/packages/expo-updates/android/src/main/java/expo/modules/updates/UpdatesUtils.java#L126

@jkhales jkhales force-pushed the jonathan/eng-1928-fileextension-android branch from 99233f3 to 8917bec Compare September 28, 2021 11:58
@jkhales jkhales merged commit 0ef76df into master Sep 28, 2021
@jkhales jkhales deleted the jonathan/eng-1928-fileextension-android branch September 28, 2021 15:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants