Skip to content

Commit

Permalink
Remove chance of blank keys and ids in the cache
Browse files Browse the repository at this point in the history 
Added blank and null key behaviour tests for remove
  • Loading branch information
@rob-baillie-ortoo
rob-baillie-ortoo committed Mar 14, 2022
1 parent 6d1013e commit c40e580
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 32 deletions.
31 changes: 18 additions & 13 deletions framework/default/ortoo-core/default/classes/fflib-extension/caching/ObjectCache.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,12 @@ public inherited sharing class ObjectCache
*/
public CacheRetrieval get( String key, Set<String> ids )
{
Contract.requires( key != null, 'get called with a null key' );
Contract.requires( String.isNotBlank( key ), 'get called with a blank key' );
Contract.requires( ids != null, 'get called with a null ids' );

for ( String thisId : ids )
{
Contract.requires( thisId != null, 'get called with a null entry in ids' );
Contract.requires( String.isNotBlank( thisId ), 'get called with a blank entry in ids' );
}

CacheRetrieval values = new CacheRetrieval();
Expand Down Expand Up @@ -157,7 +157,7 @@ public inherited sharing class ObjectCache
*/
public CacheRetrieval get( String key, String id )
{
Contract.requires( id != null, 'get called with a null id' );
Contract.requires( String.isNotBlank( id ), 'get called with a blank id' );

return get( key, new Set<String>{ id } );
}
Expand All @@ -171,8 +171,8 @@ public inherited sharing class ObjectCache
*/
public CacheRetrieval get( String key, Id id )
{
Contract.requires( key != null, 'get called with a null key' );
Contract.requires( id != null, 'get called with a null id' );
Contract.requires( String.isNotBlank( key ), 'get called with a blank key' );
Contract.requires( String.isNotBlank( id ), 'get called with a blank id' );

return get( key, new Set<String>{ id } );
}
Expand Down Expand Up @@ -200,7 +200,7 @@ public inherited sharing class ObjectCache
*/
public ObjectCache put( String key, String idField, List<Sobject> sobjects )
{
Contract.requires( idField != null, 'put called with a null idField' );
Contract.requires( String.isNotBlank( idField ), 'put called with a blank idField' );
return put( key, new SobjectIdGetter( idField ), sobjects );
}

Expand All @@ -219,17 +219,22 @@ public inherited sharing class ObjectCache
*/
public ObjectCache put( String key, IdGetter idGetter, List<Object> objects )
{
Contract.requires( key != null, 'put called with a null key' );
Contract.requires( String.isNotBlank( key ), 'put called with a blank key' );
Contract.requires( idGetter != null, 'put called with a null idGetter' );
Contract.requires( objects != null, 'put called with a null objects' );

for ( Object thisObject : objects )
{
Contract.assert( thisObject != null, 'put called with a null entry in objects' );
String thisId = idGetter.getIdFor( thisObject );
Contract.requires( thisObject != null, 'put called with a null entry in objects' );

Contract.assert( thisId != null, 'put called with an object that has a null Id: ' + thisObject );
String thisId = idGetter.getIdFor( thisObject );
Contract.requires( String.isNotBlank( thisId ), 'put called with an object that has a blank Id: ' + thisObject );
}

// Done in two loops so we check that everything is valid before writing anything
for ( Object thisObject : objects )
{
String thisId = idGetter.getIdFor( thisObject );
cacheWrapper.put( buildFullKey( key, thisId ), thisObject, CACHE_LIFESPAN_SECONDS );
}

Expand All @@ -244,7 +249,7 @@ public inherited sharing class ObjectCache
*/
public ObjectCache remove( String keyToRemove )
{
Contract.requires( keyToRemove != null, 'remove called with a null keyToRemove' );
Contract.requires( String.isNotBlank( keyToRemove ), 'remove called with a blank keyToRemove' );

Set<String> allExistingKeys = cacheWrapper.getKeys();

Expand Down Expand Up @@ -280,13 +285,13 @@ public inherited sharing class ObjectCache
*/
public ObjectCache remove( String key, Set<String> ids )
{
Contract.requires( key != null, 'remove called with a null key' );
Contract.requires( String.isNotBlank( key ), 'remove called with a blank key' );
Contract.requires( ids != null, 'remove called with a null ids' );

// This is two loops because we want to check all the ids before we remove anything
for ( String thisId : ids )
{
Contract.assert( thisId != null, 'remove called with a null entry in ids' );
Contract.requires( String.isNotBlank( thisId ), 'remove called with a blank entry in ids' );
}

for ( String thisId : ids )
Expand Down
8 changes: 4 additions & 4 deletions framework/default/ortoo-core/default/classes/fflib-extension/caching/OrgCache.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public inherited sharing class OrgCache implements ICacheAdaptor
*/
public Object get( String key )
{
Contract.requires( key != null, 'get called with a null key' );
Contract.requires( String.isNotBlank( key ), 'get called with a blank key' );

if ( ! hasAccessToCache )
{
Expand Down Expand Up @@ -100,7 +100,7 @@ public inherited sharing class OrgCache implements ICacheAdaptor
*/
public void put( String key, Object value, Integer lifespan )
{
Contract.requires( key != null, 'put called with a null key' );
Contract.requires( String.isNotBlank( key ), 'put called with a blank key' );
Contract.requires( lifespan != null, 'put called with a null lifespan' );
Contract.requires( value != null, 'put called with a null value' );
Contract.requires( lifespan >= 0, 'put called with a negaitve lifespan' );
Expand All @@ -127,7 +127,7 @@ public inherited sharing class OrgCache implements ICacheAdaptor
*/
public Boolean contains( String key )
{
Contract.requires( key != null, 'contains called with a null key' );
Contract.requires( String.isNotBlank( key ), 'contains called with a blank key' );

if ( ! hasAccessToCache )
{
Expand All @@ -148,7 +148,7 @@ public inherited sharing class OrgCache implements ICacheAdaptor
*/
public void remove( String key )
{
Contract.requires( key != null, 'remove called with a null key' );
Contract.requires( String.isNotBlank( key ), 'remove called with a blank key' );

if ( ! hasAccessToCache )
{
Expand Down
8 changes: 4 additions & 4 deletions framework/default/ortoo-core/default/classes/fflib-extension/caching/SessionCache.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public inherited sharing class SessionCache implements ICacheAdaptor
*/
public Object get( String key )
{
Contract.requires( key != null, 'get called with a null key' );
Contract.requires( String.isNotBlank( key ), 'get called with a blank key' );
return Cache.Session.get( key );
}

Expand All @@ -59,7 +59,7 @@ public inherited sharing class SessionCache implements ICacheAdaptor
*/
public void put( String key, Object value, Integer lifespan )
{
Contract.requires( key != null, 'put called with a null key' );
Contract.requires( String.isNotBlank( key ), 'put called with a blank key' );
Contract.requires( value != null, 'put called with a null value (call remove instead)' );
Contract.requires( lifespan != null, 'put called with a null lifespan' );
Contract.requires( lifespan >= 0, 'put called with a negative lifespan' );
Expand All @@ -76,7 +76,7 @@ public inherited sharing class SessionCache implements ICacheAdaptor
*/
public Boolean contains( String key )
{
Contract.requires( key != null, 'contains called with a null key' );
Contract.requires( String.isNotBlank( key ), 'contains called with a blank key' );

return Cache.Session.contains( key );
}
Expand All @@ -88,7 +88,7 @@ public inherited sharing class SessionCache implements ICacheAdaptor
*/
public void remove( String key )
{
Contract.requires( key != null, 'remove called with a null key' );
Contract.requires( String.isNotBlank( key ), 'remove called with a blank key' );

Cache.Session.remove( key );
}
Expand Down
83 changes: 72 additions & 11 deletions ...work/default/ortoo-core/default/classes/fflib-extension/caching/tests/ObjectCacheTest.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,23 +101,23 @@ private without sharing class ObjectCacheTest
}

@isTest
private static void get_whenGivenANullKey_throwsAnException() // NOPMD: Test method name format
private static void get_whenGivenABlankKey_throwsAnException() // NOPMD: Test method name format
{
ObjectCache cache = new ObjectCache().setScope( ObjectCache.CacheScope.SESSION );

Test.startTest();
String exceptionMessage;
try
{
ObjectCache.CacheRetrieval retrieval = cache.get( 'key', new Set<String>{ 'id1', null, 'id2' } );
ObjectCache.CacheRetrieval retrieval = cache.get( 'key', new Set<String>{ 'id1', ' ', 'id2' } );
}
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ortoo_Asserts.assertContains( 'get called with a null entry in ids', exceptionMessage, 'get, when given a null key, will throw an exception' );
ortoo_Asserts.assertContains( 'get called with a blank entry in ids', exceptionMessage, 'get, when given a blank key, will throw an exception' );
}

@isTest
Expand Down Expand Up @@ -267,7 +267,7 @@ private without sharing class ObjectCacheTest
}
Test.stopTest();

ortoo_Asserts.assertContains( 'put called with an object that has a null Id', exceptionMessage, 'put, when given SObjects and one has a null Id, will throw an exception' );
ortoo_Asserts.assertContains( 'put called with an object that has a blank Id', exceptionMessage, 'put, when given SObjects and one has a blank Id, will throw an exception' );
}

@isTest
Expand Down Expand Up @@ -325,7 +325,7 @@ private without sharing class ObjectCacheTest
}
Test.stopTest();

ortoo_Asserts.assertContains( 'Unable to retrieve the String Identifier from the field MailingLatitude from the SObject: Contact', exceptionMessage, 'put, when given a list of sobjects and an invalid field name, will throw an exception' );
ortoo_Asserts.assertContains( 'Unable to retrieve the String Identifier from the field MailingLatitude from the SObject: Contact', exceptionMessage, 'put, when given a list of sobjects and a non string field name, will throw an exception' );
}

@isTest
Expand All @@ -342,13 +342,13 @@ private without sharing class ObjectCacheTest
new Contact()
});
}
catch ( Contract.AssertException e )
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ortoo_Asserts.assertContains( 'put called with an object that has a null Id: Contact', exceptionMessage, 'put, when given a list of sobjects and an invalid field name, will throw an exception' );
ortoo_Asserts.assertContains( 'put called with an object that has a blank Id: Contact', exceptionMessage, 'put, when given a list of sobjects and one with a null Id will throw an exception' );
}

@isTest
Expand All @@ -366,13 +366,13 @@ private without sharing class ObjectCacheTest
'hit2'
});
}
catch ( Contract.AssertException e )
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ortoo_Asserts.assertContains( 'put called with an object that has a null Id: nullid', exceptionMessage, 'put, when given a list of sobjects and an invalid field name, will throw an exception' );
ortoo_Asserts.assertContains( 'put called with an object that has a blank Id: nullid', exceptionMessage, 'put, when given a list of sobjects one with a null Id, will throw an exception' );
}

@isTest
Expand Down Expand Up @@ -546,8 +546,69 @@ private without sharing class ObjectCacheTest
System.assert( true, 'remove, when given a key and Id combination that does not exist, will not throw an exception' );
}

// TODO: remove called with a null / empty id will throw an exception
// TODO: remove called with a null / empty id will not remove other things
@isTest
private static void remove_whenGivenANullKey_throwsAnException() // NOPMD: Test method name format
{
ObjectCache cache = new ObjectCache().setScope( ObjectCache.CacheScope.SESSION );

Test.startTest();
String exceptionMessage;
try
{
cache.remove( 'key', new Set<String>{ 'id1', ' ', 'id2' } );
}
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ortoo_Asserts.assertContains( 'remove called with a blank entry in ids', exceptionMessage, 'remove, when given a blank key, will throw an exception' );
}

@isTest
private static void remove_whenGivenABlankKey_throwsAnException() // NOPMD: Test method name format
{
ObjectCache cache = new ObjectCache().setScope( ObjectCache.CacheScope.SESSION );

Test.startTest();
String exceptionMessage;
try
{
cache.remove( 'key', new Set<String>{ 'id1', ' ', 'id2' } );
}
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ortoo_Asserts.assertContains( 'remove called with a blank entry in ids', exceptionMessage, 'remove, when given a blank key, will throw an exception' );
}

@isTest
private static void remove_whenGivenABlankKey_willNotRemoveTheOtherKeys()
{
ObjectCache cache = new ObjectCache().setScope( ObjectCache.CacheScope.SESSION )
.put( 'key', new StringIdGetter(), new List<String>{ 'value1', 'value2', 'value3' } );

Test.startTest();
String exceptionMessage;
try
{
cache.remove( 'key', new Set<String>{ 'value1Id', ' ', 'value3Id' } );
}
catch ( Contract.RequiresException e )
{
exceptionMessage = e.getMessage();
}
Test.stopTest();

ObjectCache.CacheRetrieval retrieval = new ObjectCache().setScope( ObjectCache.CacheScope.SESSION )
.get( 'key', new Set<String>{ 'value1Id', 'value2Id', 'value3Id' } );

System.assertEquals( 3, retrieval.cacheHits.size(), 'remove, when given a mix of blank and non-blank keys, will not remove the blank ones' );
}

public class StringIdGetter implements ObjectCache.IdGetter
{
Expand Down

0 comments on commit c40e580

Please sign in to comment.