Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Added defensive copy mechanism #282

Merged
merged 3 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions doc/src/main/markdown/further.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,12 @@ Consider the following worked example:
<!-- code_link_start -->

[flow.Unpredictable]: ../../../../api/src/main/java/com/mastercard/test/flow/Unpredictable.java
[AbstractMessage.masking(Unpredictable,UnaryOperator)]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/AbstractMessage.java#L46-L53,46-53
[AbstractMessage.masking(Unpredictable,UnaryOperator)]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/AbstractMessage.java#L48-L55,48-55
[AbstractFlocessor.masking(Unpredictable...)]: ../../../../assert/assert-core/src/main/java/com/mastercard/test/flow/assrt/AbstractFlocessor.java#L202-L209,202-209
[mask.BenSys]: ../../test/java/com/mastercard/test/flow/doc/mask/BenSys.java
[mask.DieSys]: ../../test/java/com/mastercard/test/flow/doc/mask/DieSys.java
[mask.Unpredictables]: ../../test/java/com/mastercard/test/flow/doc/mask/Unpredictables.java
[AbstractMessage.masking(Unpredictable,UnaryOperator)]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/AbstractMessage.java#L46-L53,46-53
[AbstractMessage.masking(Unpredictable,UnaryOperator)]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/AbstractMessage.java#L48-L55,48-55
[Rolling?d\+]: ../../test/java/com/mastercard/test/flow/doc/mask/Rolling.java#L30,30
[msg.Mask]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/Mask.java
[msg.Mask.andThen(Consumer)]: ../../../../message/message-core/src/main/java/com/mastercard/test/flow/msg/Mask.java#L290-L292,290-292
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.lang.reflect.Array;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
Expand All @@ -13,6 +14,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.function.UnaryOperator;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand Down Expand Up @@ -111,12 +113,33 @@ public T set( String field, Object value ) {
return self();
}

@Override
public final Object get( String field ) {
Object value = access( field );
if( value == null ) {
return null;
}
return defensiveCopiers.getOrDefault(
value.getClass(),
o -> o )
.apply( value );
}

/**
* Field accessor
*
* @param field The field address
* @return The field value
*/
protected abstract Object access( String field );

private static final Set<Class<?>> immutableTypes = Stream.of(
Boolean.class, Byte.class, Short.class, Character.class,
Integer.class, Float.class,
Long.class, Double.class,
String.class,
BigDecimal.class, BigInteger.class )
BigDecimal.class, BigInteger.class,
UUID.class )
.collect( toCollection( HashSet::new ) );

/**
Expand All @@ -141,6 +164,15 @@ protected Object validateValueType( String field, Object value ) {
}
return copy;
}
if( defensiveCopiers.containsKey( value.getClass() ) ) {
Object copy = defensiveCopiers.get( value.getClass() ).apply( value );
if( copy == value ) {
throw new IllegalStateException( String.format(
"Ineffective defensive copy for field '%s', type %s",
field, value.getClass() ) );
}
return copy;
}
if( value != DELETE
&& !value.getClass().isPrimitive()
&& !value.getClass().isEnum()
Expand All @@ -151,6 +183,9 @@ protected Object validateValueType( String field, Object value ) {
+ " AbstractMessage.registerImmutable( " + value.getClass().getSimpleName()
+ ".class )\n"
+ "to suppress this error.\n"
+ "Alternatively you can use\n"
+ " AbstractMessage.registerDefensiveCopier( type, function )\n"
+ "to make defensive copies of you mutable types.\n"
+ "You can also override\n"
+ " AbstractMessage.validateValueType( field, value )\n"
+ "in your subclass implementation to do validation more suitable\n"
Expand All @@ -173,6 +208,22 @@ public static void registerImmutable( Class<?> type ) {
immutableTypes.add( type );
}

private static final Map<Class<?>, UnaryOperator<Object>> defensiveCopiers = new HashMap<>();

/**
* @param <T> value type
* @param type value type
* @param copier function to make a defensive copy of a value
*/
@SuppressWarnings("unchecked")
public static <T> void registerDefensiveCopier( Class<T> type, UnaryOperator<T> copier ) {
defensiveCopiers.put( type, (UnaryOperator<Object>) copier );
}

static {
registerDefensiveCopier( Timestamp.class, t -> new Timestamp( t.getTime() ) );
}

/**
* @return <code>this</code>
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@

import java.lang.reflect.Array;
import java.lang.reflect.Field;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.function.UnaryOperator;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -79,8 +85,12 @@ public Set<String> fields() {
}

@Override
public Object get( String field ) {
throw new UnsupportedOperationException();
protected Object access( String field ) {
return updates.stream()
.filter( u -> u.field().equals( field ) )
.findAny()
.map( Update::value )
.orElse( null );
}

@Override
Expand Down Expand Up @@ -162,7 +172,7 @@ void copyMasksTo() {
void mutableValues() throws Exception {

// pitest runs the test multiple times in the same classloader, so we have to
// clear the static immuatble type set back to default
// clear the static immutable type set back to default
Field f = AbstractMessage.class.getDeclaredField( "immutableTypes" );
f.setAccessible( true );
((Set<Class<?>>) f.get( null )).remove( ArrayList.class );
Expand All @@ -177,6 +187,9 @@ void mutableValues() throws Exception {
+ "If you're sure that this type is immutable, then you can call\n"
+ " AbstractMessage.registerImmutable( EmptyList.class )\n"
+ "to suppress this error.\n"
+ "Alternatively you can use\n"
+ " AbstractMessage.registerDefensiveCopier( type, function )\n"
+ "to make defensive copies of you mutable types.\n"
+ "You can also override\n"
+ " AbstractMessage.validateValueType( field, value )\n"
+ "in your subclass implementation to do validation more suitable\n"
Expand All @@ -191,6 +204,109 @@ void mutableValues() throws Exception {
msg.set( "field", new ArrayList<>() );
}

/**
* Runs through the default allowed types and shows they pass the mutability
* filter
*/
@Test
void validTypes() {
ConcreteMessage msg = new ConcreteMessage( "msg" );
msg.set( "boolean", true );
msg.set( "byte", (byte) 1 );
msg.set( "short", (short) 2 );
msg.set( "character", 'a' );
msg.set( "integer", 4 );
msg.set( "float", 4.0f );
msg.set( "long", 8L );
msg.set( "double", 8.0 );
msg.set( "string", "bcd" );
msg.set( "bigdecimal", new BigDecimal( "1.234" ) );
msg.set( "biginteger", new BigInteger( "5678" ) );
msg.set( "uuid", new UUID( 12345L, 67890L ) );
msg.set( "timestamp", new Timestamp( 1234567890L ) );
msg.set( "null", null );

assertEquals( true, msg.get( "boolean" ) );
assertEquals( (byte) 1, msg.get( "byte" ) );
assertEquals( (short) 2, msg.get( "short" ) );
assertEquals( 'a', msg.get( "character" ) );
assertEquals( 4, msg.get( "integer" ) );
assertEquals( 4.0f, msg.get( "float" ) );
assertEquals( 8L, msg.get( "long" ) );
assertEquals( 8.0, msg.get( "double" ) );
assertEquals( "bcd", msg.get( "string" ) );
assertEquals( new BigDecimal( "1.234" ), msg.get( "bigdecimal" ) );
assertEquals( new BigInteger( "5678" ), msg.get( "biginteger" ) );
assertEquals( new UUID( 12345L, 67890L ), msg.get( "uuid" ) );
assertEquals( new Timestamp( 1234567890L ), msg.get( "timestamp" ) );
assertEquals( null, msg.get( "null" ) );
assertEquals( null, msg.get( "no such field" ) );
}

/**
* Demonstrates the mechanism that allows mutable field types to be used via
* defensive copies
*
* @throws Exception if something goes wrong with our reflection
*/
@SuppressWarnings("unchecked")
@Test
void defensiveCopy() throws Exception {

// pitest runs the test multiple times in the same classloader, so we have to
// clear the static copier map set back to default
Field f = AbstractMessage.class.getDeclaredField( "defensiveCopiers" );
f.setAccessible( true );
((Map<Class<?>, UnaryOperator<Object>>) f.get( null )).remove( StringBuilder.class );

ConcreteMessage msg = new ConcreteMessage( "msg" );

assertThrows( IllegalArgumentException.class,
() -> msg.set( "field", new StringBuilder( "buff" ) ) );

AbstractMessage.registerDefensiveCopier( StringBuilder.class,
b -> new StringBuilder( "copy of [" ).append( b.toString() ).append( "]" ) );

msg.set( "field", new StringBuilder( "buff" ) );

assertEquals( ""
+ "msg\n"
+ " field=copy of [buff]",
msg.asHuman(),
"The message holds a defensive copy of the supplied value" );

assertEquals( "copy of [copy of [buff]]", msg.get( "field" ).toString(),
"When queried, the message returns a defensive copy of the held value" );
}

/**
* Demonstrates that we reject ineffective defensive copiers
*
* @throws Exception if something goes wrong with our reflection
*/
@SuppressWarnings("unchecked")
@Test
void badDefensiveCopy() throws Exception {

// pitest runs the test multiple times in the same classloader, so we have to
// clear the static copier map set back to default
Field f = AbstractMessage.class.getDeclaredField( "defensiveCopiers" );
f.setAccessible( true );
((Map<Class<?>, UnaryOperator<Object>>) f.get( null )).remove( StringBuilder.class );

ConcreteMessage msg = new ConcreteMessage( "msg" );

AbstractMessage.registerDefensiveCopier( StringBuilder.class,
b -> b );

IllegalStateException ise = assertThrows( IllegalStateException.class,
() -> msg.set( "field", new StringBuilder( "buff" ) ) );

assertEquals(
"Ineffective defensive copy for field 'field', type class java.lang.StringBuilder",
ise.getMessage() );
}

/**
* Shows that setting arrays takes a defensive copy
*/
Expand All @@ -214,6 +330,9 @@ void arrayDefense() {
+ "If you're sure that this type is immutable, then you can call\n"
+ " AbstractMessage.registerImmutable( Object.class )\n"
+ "to suppress this error.\n"
+ "Alternatively you can use\n"
+ " AbstractMessage.registerDefensiveCopier( type, function )\n"
+ "to make defensive copies of you mutable types.\n"
+ "You can also override\n"
+ " AbstractMessage.validateValueType( field, value )\n"
+ "in your subclass implementation to do validation more suitable\n"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ else if( isHttpField( field ) ) {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
if( BODY.equals( field ) ) {
return body.orElse( null );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ else if( value == EMPTY_LIST ) {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
AtomicReference<Object> result = new AtomicReference<>();
traverse( data(), field, false,
( map, key ) -> result.set( map.get( key ) ),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public Set<String> fields() {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
return data().get( field );
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public Set<String> fields() {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
ResultSetStructure data = data();
if( COLUMNS.equals( field ) ) {
return new ArrayList<>( data.columns );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ protected String asHuman() {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
Matcher m = Pattern.compile( field ).matcher( build() );
if( m.find() ) {
return m.groupCount() == 0 ? m.group( 0 ) : m.group( 1 );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public WebSequence operation( String name,
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
return parameters().get( field );
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ else if( value instanceof List ) {
}

@Override
public Object get( String field ) {
protected Object access( String field ) {
AtomicReference<Object> result = new AtomicReference<>();
traverse( data(), field, false, false,
( map, key ) -> result.set( map.get( key ) ),
Expand Down