Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,15 @@
* that a certain key is mapped to a particular value <em>or</em> that there are no mapping for
* the key in the map.
*
* <p>This map is implemented as a Merkle-Patricia tree. It does not permit null keys and values,
* and requires that keys are 32-byte long.
* <p>This map is implemented as a Merkle-Patricia tree. It does not permit null keys and values.
*
* <p>This map can be instantiated in two ways - either with keys hashing with methods
Copy link
Contributor

Choose a reason for hiding this comment

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

with..with...
how about : ...either with keys hashing using methods...

* {@link #newInstance(String, View, Serializer, Serializer)} and
* {@link #newInGroupUnsafe(String, byte[], View, Serializer, Serializer)} or with no key hashing
* with methods {@link #newInstanceNoKeyHashing(String, View, Serializer, Serializer)} and
* {@link #newInGroupUnsafeNoKeyHashing(String, byte[], View, Serializer, Serializer)}. In case of
* no key hashing keys are required to be 32-byte long. Note that the former option is considered
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed why it shall be the default option, and the documentation must properly reflect that.

* a default one.
*
* <p>The "destructive" methods of the map, i.e., the one that change the map contents,
* are specified to throw {@link UnsupportedOperationException} if
Expand All @@ -58,7 +65,7 @@
* <p>When the view goes out of scope, this map is destroyed. Subsequent use of the closed map
* is prohibited and will result in {@link IllegalStateException}.
*
* @param <K> the type of keys in this map. Must be 32-byte long in the serialized form
* @param <K> the type of keys in this map
* @param <V> the type of values in this map
* @see View
*/
Expand All @@ -74,7 +81,7 @@ public final class ProofMapIndexProxy<K, V> extends AbstractIndexProxy implement
* [a-zA-Z0-9_]
* @param view a database view. Must be valid.
* If a view is read-only, "destructive" operations are not permitted.
* @param keySerializer a serializer of keys, must always produce 32-byte long values
* @param keySerializer a serializer of keys
* @param valueSerializer a serializer of values
* @param <K> the type of keys in the map
* @param <V> the type of values in the map
Expand All @@ -86,9 +93,34 @@ public static <K, V> ProofMapIndexProxy<K, V> newInstance(
String name, View view, Serializer<K> keySerializer, Serializer<V> valueSerializer) {
IndexAddress address = IndexAddress.valueOf(name);
long viewNativeHandle = view.getViewNativeHandle();
LongSupplier nativeMapConstructor = () -> nativeCreate(name, viewNativeHandle);
LongSupplier nativeMapConstructor = () -> nativeCreate(name, viewNativeHandle, true);

return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor, true);
}

/**
* Creates a ProofMapIndexProxy with no key hashing. Requires that keys are 32-byte long.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add a short warning with a link to the class-level section?

*
* @param name a unique alphanumeric non-empty identifier of this map in the underlying storage:
* [a-zA-Z0-9_]
* @param view a database view. Must be valid.
* If a view is read-only, "destructive" operations are not permitted.
* @param keySerializer a serializer of keys, must always produce 32-byte long values
* @param valueSerializer a serializer of values
* @param <K> the type of keys in the map
* @param <V> the type of values in the map
* @throws IllegalStateException if the view is not valid
* @throws IllegalArgumentException if the name is empty
* @see StandardSerializers
* @see ProofMapIndexProxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, that's this class already.

*/
public static <K, V> ProofMapIndexProxy<K, V> newInstanceNoKeyHashing(
String name, View view, Serializer<K> keySerializer, Serializer<V> valueSerializer) {
IndexAddress address = IndexAddress.valueOf(name);
long viewNativeHandle = view.getViewNativeHandle();
LongSupplier nativeMapConstructor = () -> nativeCreate(name, viewNativeHandle, false);

return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor);
return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor, false);
}

/**
Expand All @@ -100,6 +132,35 @@ public static <K, V> ProofMapIndexProxy<K, V> newInstance(
* @param groupName a name of the collection group
* @param mapId an identifier of this collection in the group, see the caveats
* @param view a database view
* @param keySerializer a serializer of keys
* @param valueSerializer a serializer of values
* @param <K> the type of keys in the map
* @param <V> the type of values in the map
* @return a new map proxy
* @throws IllegalStateException if the view is not valid
* @throws IllegalArgumentException if the name or index id is empty
* @see StandardSerializers
*/
public static <K, V> ProofMapIndexProxy<K, V> newInGroupUnsafe(
String groupName, byte[] mapId, View view, Serializer<K> keySerializer,
Serializer<V> valueSerializer) {
IndexAddress address = IndexAddress.valueOf(groupName, mapId);
long viewNativeHandle = view.getViewNativeHandle();
LongSupplier nativeMapConstructor =
() -> nativeCreateInGroup(groupName, mapId, viewNativeHandle, true);

return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor, true);
}

/**
* Creates a new proof map in a <a href="package-summary.html#families">collection group</a>
* with the given name. Requires that keys are 32-byte long.
*
* <p>See a <a href="package-summary.html#families-limitations">caveat</a> on index identifiers.
*
* @param groupName a name of the collection group
* @param mapId an identifier of this collection in the group, see the caveats
* @param view a database view
* @param keySerializer a serializer of keys, must always produce 32-byte long values
* @param valueSerializer a serializer of values
* @param <K> the type of keys in the map
Expand All @@ -108,27 +169,26 @@ public static <K, V> ProofMapIndexProxy<K, V> newInstance(
* @throws IllegalStateException if the view is not valid
* @throws IllegalArgumentException if the name or index id is empty
* @see StandardSerializers
* @see ProofMapIndexProxy
*/
public static <K, V> ProofMapIndexProxy<K, V> newInGroupUnsafe(String groupName,
byte[] mapId,
View view,
Serializer<K> keySerializer,
Serializer<V> valueSerializer) {
public static <K, V> ProofMapIndexProxy<K, V> newInGroupUnsafeNoKeyHashing(
String groupName, byte[] mapId, View view, Serializer<K> keySerializer,
Serializer<V> valueSerializer) {
IndexAddress address = IndexAddress.valueOf(groupName, mapId);
long viewNativeHandle = view.getViewNativeHandle();
LongSupplier nativeMapConstructor =
() -> nativeCreateInGroup(groupName, mapId, viewNativeHandle);
() -> nativeCreateInGroup(groupName, mapId, viewNativeHandle, false);

return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor);
return getOrCreate(address, view, keySerializer, valueSerializer, nativeMapConstructor, false);
}

private static <K, V> ProofMapIndexProxy<K, V> getOrCreate(IndexAddress address, View view,
Serializer<K> keySerializer, Serializer<V> valueSerializer,
LongSupplier nativeMapConstructor) {
LongSupplier nativeMapConstructor, boolean keyHashing) {
return view.findOpenIndex(address)
.map(ProofMapIndexProxy::<K, V>checkCachedInstance)
.orElseGet(() -> newMapIndexProxy(address, view, keySerializer, valueSerializer,
nativeMapConstructor));
nativeMapConstructor, keyHashing));
}

@SuppressWarnings("unchecked") // The compiler is correct: the cache is not type-safe: ECR-3387
Expand All @@ -139,9 +199,8 @@ private static <K, V> ProofMapIndexProxy<K, V> checkCachedInstance(StorageIndex

private static <K, V> ProofMapIndexProxy<K, V> newMapIndexProxy(IndexAddress address, View view,
Serializer<K> keySerializer, Serializer<V> valueSerializer,
LongSupplier nativeMapConstructor) {
ProofMapKeyCheckingSerializerDecorator<K> ks =
ProofMapKeyCheckingSerializerDecorator.from(keySerializer);
LongSupplier nativeMapConstructor, boolean keyHashing) {
ProofMapKeyCheckingSerializerDecorator<K> ks = decorateKeySerializer(keySerializer, keyHashing);
CheckingSerializerDecorator<V> vs = CheckingSerializerDecorator.from(valueSerializer);

NativeHandle mapNativeHandle = createNativeMap(view, nativeMapConstructor);
Expand All @@ -151,6 +210,17 @@ private static <K, V> ProofMapIndexProxy<K, V> newMapIndexProxy(IndexAddress add
return map;
}

private static <K> ProofMapKeyCheckingSerializerDecorator<K> decorateKeySerializer(
Serializer<K> keySerializer, boolean keyHashing) {
if (!keyHashing) {
ProofMapKeySizeCheckingSerializerDecorator<K> sizeCheckingSerializerDecorator =
ProofMapKeySizeCheckingSerializerDecorator.from(keySerializer);
return ProofMapKeyCheckingSerializerDecorator.from(sizeCheckingSerializerDecorator);
} else {
return ProofMapKeyCheckingSerializerDecorator.from(keySerializer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed at all if it can return just keySerializer, with no extra decorator types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To check for not null values?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is one for that already.

Copy link
Contributor

Choose a reason for hiding this comment

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

com.exonum.binding.common.serialization.CheckingSerializerDecorator

}
}

private static NativeHandle createNativeMap(View view, LongSupplier nativeMapConstructor) {
NativeHandle mapNativeHandle = new NativeHandle(nativeMapConstructor.getAsLong());

Expand All @@ -160,10 +230,10 @@ private static NativeHandle createNativeMap(View view, LongSupplier nativeMapCon
return mapNativeHandle;
}

private static native long nativeCreate(String name, long viewNativeHandle);
private static native long nativeCreate(String name, long viewNativeHandle, boolean keyHashing);

private static native long nativeCreateInGroup(String groupName, byte[] mapId,
long viewNativeHandle);
long viewNativeHandle, boolean keyHashing);

private ProofMapIndexProxy(NativeHandle nativeHandle, IndexAddress address, View view,
ProofMapKeyCheckingSerializerDecorator<K> keySerializer,
Expand All @@ -184,10 +254,11 @@ public boolean containsKey(K key) {
/**
* {@inheritDoc}
*
* @param key a proof map key, must be 32-byte long when serialized
* @param key a proof map key
* @param value a storage value to associate with the key
* @throws IllegalStateException if this map is not valid
* @throws IllegalArgumentException if the size of the key is not 32 bytes
* @throws IllegalArgumentException if the size of the key is not 32 bytes (in case of a
* no key hashing proof map)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere 'a no key hashing proof map' does not sound correct.

  • ... a non-key-hashing proof map
  • ... a proof map that does not hash keys
  • if the size of the key is not 32 bytes and this map uses non-hashed keys ...

Copy link
Contributor

Choose a reason for hiding this comment

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

@Maria-Nosyk Would you tell us please which works best?

Copy link
Contributor

Choose a reason for hiding this comment

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

The third variant is absolutely perfect!

Copy link
Contributor

Choose a reason for hiding this comment

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

Brilliant, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is non-key-hashing proof map a bad option though? Third variant is a bit too wordy in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, the construction you suggest is, first, hard to understand, second, contradicts general requirements to technical documentation. The second variant suggested by @dmitry-timofeev is also acceptable.

* @throws UnsupportedOperationException if this map is read-only
*/
@Override
Expand Down Expand Up @@ -227,11 +298,11 @@ public V get(K key) {
* Returns a proof that there are values mapped to the specified keys or that there are no such
* mappings.
*
* @param key a proof map key which might be mapped to some value, must be 32-byte long
* @param otherKeys other proof map keys which might be mapped to some values, each must be
* 32-byte long
* @param key a proof map key which might be mapped to some value
* @param otherKeys other proof map keys which might be mapped to some values
* @throws IllegalStateException if this map is not valid
* @throws IllegalArgumentException if the size of any of the keys is not 32 bytes
* @throws IllegalArgumentException if the size of any of the keys is not 32 bytes (in case of a
* no key hashing proof map)
*/
public UncheckedMapProof getProof(K key, K... otherKeys) {
if (otherKeys.length == 0) {
Expand All @@ -246,10 +317,10 @@ public UncheckedMapProof getProof(K key, K... otherKeys) {
* Returns a proof that there are values mapped to the specified keys or that there are no such
* mappings.
*
* @param keys proof map keys which might be mapped to some values, each must be 32-byte long
* @param keys proof map keys which might be mapped to some values
* @throws IllegalStateException if this map is not valid
* @throws IllegalArgumentException if the size of any of the keys is not 32 bytes
* or keys collection is empty
* @throws IllegalArgumentException if the size of any of the keys is not 32 bytes (in case of a
* no key hashing proof map) or keys collection is empty
*/
public UncheckedMapProof getProof(Collection<? extends K> keys) {
checkArgument(!keys.isEmpty(), "Keys collection should not be empty");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,12 @@

package com.exonum.binding.core.storage.indices;

import static com.exonum.binding.core.storage.indices.StoragePreconditions.checkProofKey;
import static com.google.common.base.Preconditions.checkNotNull;

import com.exonum.binding.common.serialization.Serializer;

/**
* A serializer decorator that checks proof map keys for correctness.
*
* @see StoragePreconditions#checkProofKey(byte[])
* A serializer decorator that checks proof map keys are not null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert the class to its past form.

*/
final class ProofMapKeyCheckingSerializerDecorator<T> implements Serializer<T> {

Expand All @@ -49,12 +46,12 @@ private ProofMapKeyCheckingSerializerDecorator(Serializer<T> delegate) {
@Override
public byte[] toBytes(T proofKey) {
byte[] dbValue = delegate.toBytes(proofKey);
return checkProofKey(dbValue);
return checkNotNull(dbValue);
}

@Override
public T fromBytes(byte[] serializedProofKey) {
checkProofKey(serializedProofKey);
checkNotNull(serializedProofKey);
return delegate.fromBytes(serializedProofKey);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright 2019 The Exonum Team
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.exonum.binding.core.storage.indices;

import static com.exonum.binding.core.storage.indices.StoragePreconditions.checkProofKey;
import static com.google.common.base.Preconditions.checkNotNull;

import com.exonum.binding.common.serialization.Serializer;

/**
* A serializer decorator that checks that proof map keys are 32-byte long.
*
* @see StoragePreconditions#checkProofKey(byte[])
*/
final class ProofMapKeySizeCheckingSerializerDecorator<T> implements Serializer<T> {

private final Serializer<T> delegate;

/**
* Creates a proof map key checking serializer decorator. Will not decorate itself.
*
* @param serializer a serializer to decorate
*/
public static <T> ProofMapKeySizeCheckingSerializerDecorator<T> from(Serializer<T> serializer) {
if (serializer instanceof ProofMapKeySizeCheckingSerializerDecorator) {
return (ProofMapKeySizeCheckingSerializerDecorator<T>) serializer;
}
return new ProofMapKeySizeCheckingSerializerDecorator<>(serializer);
}

private ProofMapKeySizeCheckingSerializerDecorator(Serializer<T> delegate) {
this.delegate = checkNotNull(delegate);
}

@Override
public byte[] toBytes(T proofKey) {
byte[] dbValue = delegate.toBytes(proofKey);
return checkProofKey(dbValue);
}

@Override
public T fromBytes(byte[] serializedProofKey) {
checkProofKey(serializedProofKey);
return delegate.fromBytes(serializedProofKey);
}
}
Loading