diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java index 8e0943916a0..5199156393d 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java @@ -3,6 +3,7 @@ package com.facebook.react.tests.core; import static org.fest.assertions.api.Assertions.assertThat; import androidx.test.runner.AndroidJUnit4; +import com.facebook.react.bridge.NoSuchKeyException; import com.facebook.react.bridge.UnexpectedNativeTypeException; import com.facebook.react.bridge.WritableNativeArray; import com.facebook.react.bridge.WritableNativeMap; @@ -27,7 +28,6 @@ public class WritableNativeMapTest { mMap.putMap("map", new WritableNativeMap()); mMap.putArray("array", new WritableNativeArray()); mMap.putBoolean("dvacca", true); - mMap.setUseNativeAccessor(true); } @Test @@ -90,14 +90,13 @@ public class WritableNativeMapTest { mMap.getArray("string"); } - @Ignore("Needs to be implemented") @Test public void testErrorMessageContainsKey() { String key = "fkg"; try { mMap.getString(key); - Assert.fail("Expected an UnexpectedNativeTypeException to be thrown"); - } catch (UnexpectedNativeTypeException e) { + Assert.fail("Expected an NoSuchKeyException to be thrown"); + } catch (NoSuchKeyException e) { assertThat(e.getMessage()).contains(key); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java index cbd28320503..98a697978fb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java @@ -36,20 +36,14 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { private @Nullable HashMap mLocalTypeMap; private static int mJniCallCounter; - public static void setUseNativeAccessor(boolean useNativeAccessor) { - ReactFeatureFlags.useMapNativeAccessor = useNativeAccessor; - } - public static int getJNIPassCounter() { return mJniCallCounter; } private HashMap getLocalMap() { - // Fast return for the common case if (mLocalMap != null) { return mLocalMap; } - // Check and when necessary get keys atomically synchronized (this) { if (mKeys == null) { mKeys = Assertions.assertNotNull(importKeys()); @@ -73,11 +67,9 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { private native Object[] importValues(); private @Nonnull HashMap getLocalTypeMap() { - // Fast and non-blocking return for common case if (mLocalTypeMap != null) { return mLocalTypeMap; } - // Check and when necessary get keys synchronized (this) { if (mKeys == null) { mKeys = Assertions.assertNotNull(importKeys()); @@ -101,29 +93,17 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { @Override public boolean hasKey(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return hasKeyNative(name); - } return getLocalMap().containsKey(name); } - private native boolean hasKeyNative(String name); - @Override public boolean isNull(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return isNullNative(name); - } if (getLocalMap().containsKey(name)) { return getLocalMap().get(name) == null; } throw new NoSuchKeyException(name); } - private native boolean isNullNative(@Nonnull String name); - private @Nonnull Object getValue(@Nonnull String name) { if (hasKey(name) && !(isNull(name))) { return Assertions.assertNotNull(getLocalMap().get(name)); @@ -152,7 +132,7 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { private void checkInstance(String name, Object value, Class type) { if (value != null && !type.isInstance(value)) { - throw new ClassCastException( + throw new UnexpectedNativeTypeException( "Value for " + name + " cannot be cast from " @@ -164,86 +144,43 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { @Override public boolean getBoolean(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getBooleanNative(name); - } return getValue(name, Boolean.class).booleanValue(); } - private native boolean getBooleanNative(String name); - @Override public double getDouble(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getDoubleNative(name); - } return getValue(name, Double.class).doubleValue(); } - private native double getDoubleNative(String name); - @Override public int getInt(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getIntNative(name); - } - // All numbers coming out of native are doubles, so cast here then truncate return getValue(name, Double.class).intValue(); } - private native int getIntNative(String name); - @Override public @Nullable String getString(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getStringNative(name); - } return getNullableValue(name, String.class); } - private native String getStringNative(String name); - @Override public @Nullable ReadableArray getArray(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getArrayNative(name); - } return getNullableValue(name, ReadableArray.class); } - private native ReadableNativeArray getArrayNative(String name); - @Override public @Nullable ReadableNativeMap getMap(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getMapNative(name); - } return getNullableValue(name, ReadableNativeMap.class); } - private native ReadableNativeMap getMapNative(String name); - @Override public @Nonnull ReadableType getType(@Nonnull String name) { - if (ReactFeatureFlags.useMapNativeAccessor) { - mJniCallCounter++; - return getTypeNative(name); - } if (getLocalTypeMap().containsKey(name)) { return Assertions.assertNotNull(getLocalTypeMap().get(name)); } throw new NoSuchKeyException(name); } - private native ReadableType getTypeNative(String name); - @Override public @Nonnull Dynamic getDynamic(@Nonnull String name) { return DynamicFromMap.create(this, name); @@ -275,42 +212,6 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { @Override public @Nonnull HashMap toHashMap() { - if (ReactFeatureFlags.useMapNativeAccessor) { - ReadableMapKeySetIterator iterator = keySetIterator(); - HashMap hashMap = new HashMap<>(); - - while (iterator.hasNextKey()) { - // increment for hasNextKey call - mJniCallCounter++; - String key = iterator.nextKey(); - // increment for nextKey call - mJniCallCounter++; - switch (getType(key)) { - case Null: - hashMap.put(key, null); - break; - case Boolean: - hashMap.put(key, getBoolean(key)); - break; - case Number: - hashMap.put(key, getDouble(key)); - break; - case String: - hashMap.put(key, getString(key)); - break; - case Map: - hashMap.put(key, Assertions.assertNotNull(getMap(key)).toHashMap()); - break; - case Array: - hashMap.put(key, Assertions.assertNotNull(getArray(key)).toArrayList()); - break; - default: - throw new IllegalArgumentException("Could not convert object with key: " + key + "."); - } - } - return hashMap; - } - // we can almost just return getLocalMap(), but we need to convert nested arrays and maps to the // correct types first HashMap hashMap = new HashMap<>(getLocalMap()); @@ -337,25 +238,21 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap { return hashMap; } - /** Implementation of a {@link ReadableNativeMap} iterator in native memory. */ - @DoNotStrip private static class ReadableNativeMapKeySetIterator implements ReadableMapKeySetIterator { - @DoNotStrip private final HybridData mHybridData; - - // Need to hold a strong ref to the map so that our native references remain valid. - @DoNotStrip private final ReadableNativeMap mMap; + private final Iterator mIterator; public ReadableNativeMapKeySetIterator(ReadableNativeMap readableNativeMap) { - mMap = readableNativeMap; - mHybridData = initHybrid(readableNativeMap); + mIterator = readableNativeMap.getLocalMap().keySet().iterator(); } @Override - public native boolean hasNextKey(); + public boolean hasNextKey() { + return mIterator.hasNext(); + } @Override - public native String nextKey(); - - private static native HybridData initHybrid(ReadableNativeMap readableNativeMap); + public String nextKey() { + return mIterator.next(); + } } } diff --git a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp index 3767097d574..c754eb12b16 100644 --- a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp @@ -76,7 +76,6 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) { NativeMap::registerNatives(); ReadableNativeMap::registerNatives(); WritableNativeMap::registerNatives(); - ReadableNativeMapKeySetIterator::registerNatives(); #ifdef WITH_INSPECTOR JInspector::registerNatives(); diff --git a/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp b/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp index 41f6f2471d2..3a09e29692c 100644 --- a/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp +++ b/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp @@ -12,6 +12,31 @@ using namespace facebook::jni; namespace facebook { namespace react { +jint makeJIntOrThrow(int64_t integer) { + jint javaint = static_cast(integer); + if (integer != javaint) { + throwNewJavaException( + exceptions::gUnexpectedNativeTypeExceptionClass, + "Value '%lld' doesn't fit into a 32 bit signed int", integer); + } + return javaint; +} + +int64_t convertDynamicIfIntegral(const folly::dynamic& val) { + if (val.isInt()) { + return val.getInt(); + } + double dbl = val.getDouble(); + int64_t result = static_cast(dbl); + if (dbl != result) { + throwNewJavaException( + exceptions::gUnexpectedNativeTypeExceptionClass, + "Tried to read an int, but got a non-integral double: %f", dbl); + } + return result; +} + + // This attribute exports the ctor symbol, so ReadableNativeArray to be // constructed from other DSOs. diff --git a/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp b/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp index 8d395b3b092..980c7086aea 100644 --- a/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp +++ b/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp @@ -10,10 +10,6 @@ using namespace facebook::jni; namespace facebook { namespace react { -namespace { -const char *gNoSuchKeyExceptionClass = "com/facebook/react/bridge/NoSuchKeyException"; -} // namespace - void ReadableNativeMap::mapException(const std::exception& ex) { if (dynamic_cast(&ex) != nullptr) { throwNewJavaException(exceptions::gUnexpectedNativeTypeExceptionClass, ex.what()); @@ -28,8 +24,8 @@ local_ref> ReadableNativeMap::importKeys() { } jint size = keys_.value().size(); auto jarray = JArrayClass::newArray(size); - for (jint i = 0; i < size; i++) { - (*jarray)[i] = make_jstring(keys_.value()[i].getString()); + for (jint ii = 0; ii < size; ii++) { + (*jarray)[ii] = make_jstring(keys_.value()[ii].getString()); } return jarray; } @@ -37,39 +33,40 @@ local_ref> ReadableNativeMap::importKeys() { local_ref> ReadableNativeMap::importValues() { jint size = keys_.value().size(); auto jarray = JArrayClass::newArray(size); - for (jint i = 0; i < size; i++) { - std::string key = keys_.value()[i].getString().c_str(); - const auto element = map_.at(key); + for (jint ii = 0; ii < size; ii++) { + const std::string &key = keys_.value()[ii].getString(); + const auto &element = map_.at(key); switch(element.type()) { case folly::dynamic::Type::NULLT: { - jarray->setElement(i, nullptr); + jarray->setElement(ii, nullptr); break; } case folly::dynamic::Type::BOOL: { - (*jarray)[i] = - JBoolean::valueOf(ReadableNativeMap::getBooleanKey(key)); + (*jarray)[ii] = JBoolean::valueOf(element.getBool()); + break; + } + case folly::dynamic::Type::INT64: { + (*jarray)[ii] = JDouble::valueOf(element.getInt()); break; } - case folly::dynamic::Type::INT64: case folly::dynamic::Type::DOUBLE: { - (*jarray)[i] = - JDouble::valueOf(ReadableNativeMap::getDoubleKey(key)); + (*jarray)[ii] = JDouble::valueOf(element.getDouble()); break; } case folly::dynamic::Type::STRING: { - (*jarray)[i] = ReadableNativeMap::getStringKey(key); + (*jarray)[ii] = make_jstring(element.getString()); break; } case folly::dynamic::Type::OBJECT: { - (*jarray)[i] = ReadableNativeMap::getMapKey(key); + (*jarray)[ii] = ReadableNativeMap::newObjectCxxArgs(element); break; } case folly::dynamic::Type::ARRAY: { - (*jarray)[i] = ReadableNativeMap::getArrayKey(key); + (*jarray)[ii] = ReadableNativeArray::newObjectCxxArgs(element); break; } default: { - jarray->setElement(i,nullptr); + jarray->setElement(ii, nullptr); break; } } @@ -80,80 +77,13 @@ local_ref> ReadableNativeMap::importValues() { local_ref> ReadableNativeMap::importTypes() { jint size = keys_.value().size(); auto jarray = JArrayClass::newArray(size); - for (jint i = 0; i < size; i++) { - std::string key = keys_.value()[i].getString().c_str(); - (*jarray)[i] = ReadableNativeMap::getValueType(key); + for (jint ii = 0; ii < size; ii++) { + const std::string &key = keys_.value()[ii].getString(); + (*jarray)[ii] = ReadableType::getType(map_.at(key).type()); } return jarray; } -bool ReadableNativeMap::hasKey(const std::string& key) { - return map_.find(key) != map_.items().end(); -} - -const folly::dynamic& ReadableNativeMap::getMapValue(const std::string& key) { - try { - return map_.at(key); - } catch (const std::out_of_range& ex) { - throwNewJavaException(gNoSuchKeyExceptionClass, ex.what()); - } -} - -bool ReadableNativeMap::isNull(const std::string& key) { - return getMapValue(key).isNull(); -} - -bool ReadableNativeMap::getBooleanKey(const std::string& key) { - return getMapValue(key).getBool(); -} - -double ReadableNativeMap::getDoubleKey(const std::string& key) { - const folly::dynamic& val = getMapValue(key); - if (val.isInt()) { - return val.getInt(); - } - return val.getDouble(); -} - -jint ReadableNativeMap::getIntKey(const std::string& key) { - const folly::dynamic& val = getMapValue(key); - int64_t integer = convertDynamicIfIntegral(val); - return makeJIntOrThrow(integer); -} - -local_ref ReadableNativeMap::getStringKey(const std::string& key) { - const folly::dynamic& val = getMapValue(key); - if (val.isNull()) { - return local_ref(nullptr); - } - return make_jstring(val.getString().c_str()); -} - -local_ref ReadableNativeMap::getArrayKey(const std::string& key) { - auto& value = getMapValue(key); - if (value.isNull()) { - return local_ref(nullptr); - } else { - return ReadableNativeArray::newObjectCxxArgs(value); - } -} - -local_ref ReadableNativeMap::getMapKey(const std::string& key) { - auto& value = getMapValue(key); - if (value.isNull()) { - return local_ref(nullptr); - } else if (!value.isObject()) { - throwNewJavaException(exceptions::gUnexpectedNativeTypeExceptionClass, - "expected Map, got a %s", value.typeName()); - } else { - return ReadableNativeMap::newObjectCxxArgs(value); - } -} - -local_ref ReadableNativeMap::getValueType(const std::string& key) { - return ReadableType::getType(getMapValue(key).type()); -} - local_ref ReadableNativeMap::createWithContents(folly::dynamic&& map) { if (map.isNull()) { return local_ref(nullptr); @@ -172,71 +102,8 @@ void ReadableNativeMap::registerNatives() { makeNativeMethod("importKeys", ReadableNativeMap::importKeys), makeNativeMethod("importValues", ReadableNativeMap::importValues), makeNativeMethod("importTypes", ReadableNativeMap::importTypes), - makeNativeMethod("hasKeyNative", ReadableNativeMap::hasKey), - makeNativeMethod("isNullNative", ReadableNativeMap::isNull), - makeNativeMethod("getBooleanNative", ReadableNativeMap::getBooleanKey), - makeNativeMethod("getDoubleNative", ReadableNativeMap::getDoubleKey), - makeNativeMethod("getIntNative", ReadableNativeMap::getIntKey), - makeNativeMethod("getStringNative", ReadableNativeMap::getStringKey), - makeNativeMethod("getArrayNative", ReadableNativeMap::getArrayKey), - makeNativeMethod("getMapNative", ReadableNativeMap::getMapKey), - makeNativeMethod("getTypeNative", ReadableNativeMap::getValueType), }); } -ReadableNativeMapKeySetIterator::ReadableNativeMapKeySetIterator(const folly::dynamic& map) - : iter_(map.items().begin()) - , map_(map) {} - -local_ref ReadableNativeMapKeySetIterator::initHybrid(alias_ref, ReadableNativeMap* nativeMap) { - return makeCxxInstance(nativeMap->map_); -} - -bool ReadableNativeMapKeySetIterator::hasNextKey() { - return iter_ != map_.items().end(); -} - -local_ref ReadableNativeMapKeySetIterator::nextKey() { - if (!hasNextKey()) { - throwNewJavaException("com/facebook/react/bridge/InvalidIteratorException", - "No such element exists"); - } - auto ret = make_jstring(iter_->first.c_str()); - ++iter_; - return ret; -} - -void ReadableNativeMapKeySetIterator::registerNatives() { - registerHybrid({ - makeNativeMethod("hasNextKey", ReadableNativeMapKeySetIterator::hasNextKey), - makeNativeMethod("nextKey", ReadableNativeMapKeySetIterator::nextKey), - makeNativeMethod("initHybrid", ReadableNativeMapKeySetIterator::initHybrid), - }); -} - -jint makeJIntOrThrow(int64_t integer) { - jint javaint = static_cast(integer); - if (integer != javaint) { - throwNewJavaException( - exceptions::gUnexpectedNativeTypeExceptionClass, - "Value '%lld' doesn't fit into a 32 bit signed int", integer); - } - return javaint; -} - -int64_t convertDynamicIfIntegral(const folly::dynamic& val) { - if (val.isInt()) { - return val.getInt(); - } - double dbl = val.getDouble(); - int64_t result = static_cast(dbl); - if (dbl != result) { - throwNewJavaException( - exceptions::gUnexpectedNativeTypeExceptionClass, - "Tried to read an int, but got a non-integral double: %f", dbl); - } - return result; -} - } // namespace react } // namespace facebook diff --git a/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h b/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h index 98da7a3842a..57511fad2bc 100644 --- a/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h +++ b/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h @@ -29,16 +29,6 @@ struct ReadableNativeMap : jni::HybridClass { jni::local_ref> importKeys(); jni::local_ref> importValues(); jni::local_ref> importTypes(); - bool hasKey(const std::string& key); - const folly::dynamic& getMapValue(const std::string& key); - bool isNull(const std::string& key); - bool getBooleanKey(const std::string& key); - double getDoubleKey(const std::string& key); - jint getIntKey(const std::string& key); - jni::local_ref getStringKey(const std::string& key); - jni::local_ref getArrayKey(const std::string& key); - jni::local_ref getMapKey(const std::string& key); - jni::local_ref getValueType(const std::string& key); folly::Optional keys_; static jni::local_ref createWithContents(folly::dynamic&& map); @@ -51,24 +41,5 @@ struct ReadableNativeMap : jni::HybridClass { friend struct WritableNativeMap; }; -struct ReadableNativeMapKeySetIterator : jni::HybridClass { - static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/ReadableNativeMap$ReadableNativeMapKeySetIterator;"; - - ReadableNativeMapKeySetIterator(const folly::dynamic& map); - - bool hasNextKey(); - jni::local_ref nextKey(); - - static jni::local_ref initHybrid(jni::alias_ref, ReadableNativeMap* nativeMap); - static void registerNatives(); - - folly::dynamic::const_item_iterator iter_; - // The Java side holds a strong ref to the Java ReadableNativeMap. - const folly::dynamic& map_; -}; - -jint makeJIntOrThrow(int64_t integer); -int64_t convertDynamicIfIntegral(const folly::dynamic&); - } // namespace react } // namespace facebook