To Steve's point on my PR in iOS, should we add a call to the handler here with false if eventHistory is null? And similarly for getHistoricalEvents too: https://github.com/adobe/aepsdk-core-ios/pull/1121/files/166478db789731592a63b0e58bf4fa950b84eaf1#r2017691177

Forgot to remove this in mine, but with the removal of keys support, I think we can remove:
EVENT_HISTORY_TOKEN_PREFIX and EVENT_HISTORY_KEYS_KEY

Doesn't getTypedMap throw on cast failure? The Kotlin side would need to handle any throws right?:

Just an idea but something along the lines of:

val content: Map<String, Any>?
try {
    content = DataReader.getTypedMap(Any::class.java, schemaData, EVENT_HISTORY_CONTENT_KEY)
} catch (e: DataReaderException) {
    Log.warning(
        LaunchRulesEngineConstants.LOG_TAG,
        logTag,
        "Unable to process eventHistoryOperation operation for consequence ${consequence.id}, 'content' is either missing or improperly formatted in 'details.data': ${e.localizedMessage}"
    )
    return
}

Good catch! I will change to use optTypedMap and return null so the check on the next line causes a fast fail

This cast with potential throw also needs to be handled right?

I will change it to use optTypedMap since we already have a null check for detailData

Sorry not sure if this is covered already somewhere else, but could we also add a case that tests when recording an event without a mask applied?

I think this is out of scope for this PR, but I noticed that there is a logic difference between iOS and Android in general for the fnv1a32 method:

• Android checks that the mask is not null OR empty
• iOS only checks that the mask is not null: https://github.com/timkimadobe/aepsdk-core-ios/blob/872b423b3d7767839b050cdfb9eced9ee6480a2c/AEPCore/Sources/eventhub/history/EventData%2BFNV1A32.swift#L37

Looking at the history for Android changes, it has been like this for a while, even before the Kotlin conversion:

• See StringEncoder: 0856ec4#diff-e14f70bc3f52699f03f05586866ab0379f20735ec07985433623a11fd0e674bb

It's a breaking change to reconcile them, but should we make them consistent?

I changed the implementation in the newly added toEventHistoryRequest extension function. Discussing offline about changing the existing fnv1a32 Map extension function since its a breaking change

Changed the MapExtension. fnv1a32 to not write to event history if mask is empty map

Does this wrapper for querying historical conditions help here?

Woah did not know this existed! Thanks Prashanth

Same, thanks! Will replace it
On closer inspection the linked wrapper function has a limitation where in case of a database look up exception, the method returns 0 instead of throwing the exception. The insertIfNotExists logic would understand the 0 as event not existing in history and go ahead with the insertion, which is the opposite of what we want. Will leave the logic as is and create a Github issue to fix this wrapper function

Can this be moved to com.adobe.marketing.mobile.internal.util as EventExtensions is this only being used internally.

nit: consequence.detailId.isNullOrEmpty() .. , as we have been using kotlin extensions where available.

Since this when statement has only 2 clauses, one of which is else - transitioning to the following structure will be easier to follow.

if (!(CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT || CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT_IF_NOT_EXISTS))) {
 return;

}

if (CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT_IF_NOT_EXISTS) {
    // do some pre-reqs
}

// rest of your processing

I also implemented this as a switch in the iOS implementation with the intention of making any future cases easy to add on - would you say that this if structure handles the current case better? Would a switch be preferable if there were actually more cases? Or ifs would still be preferable?

Generally I see it this way -

• Try and not nest where possible.
• when and switch blocks are preferable for maintainability if there are multiple mutually exclusive cases.
• In case there is just one clause use a simple if/else for readability.

In this particular code, two cases are combined within a when, but still we still need an if inside to distinguish one, causing nesting on the condition on which the when was applied on.
So switching this to an early return pattern seemed cleaner because it reads linearly - Not interested in anything other than X and Y. If X, do something first. Then do shared processing for X and Y.

If we were to add more such cases, we can think of having a pre-processing step as a separate method which takes in the operation, goes through the when block on it and decides what has to be done before passing this off to actual processing.

fun handleOperation() {
    preprocess(operation)
    process(operation)
}

fun preprocess(operation) {
   when(operation) {
       L,M -> { common flow for L,M  }
       X -> { }
       Y -> { }
       else -> { }
    }
    
}


fun process(operation) {
     when(operation) {
       L -> { }
       M -> { }
       X -> { }
       Y -> { }
       else -> { }
      }
}

For sake of readability I changed it to use if-else for now

One question - instead of a outer negated OR, what if we use not equals ANDed together?

so instead of:

if (!(CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT || CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT_IF_NOT_EXISTS))) 

if (!CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT && !CONSEQUENCE_EVENT_HISTORY_OPERATION_INSERT_IF_NOT_EXISTS))) 

I feel like the intent could be more intuitive when read?

Recommend switching to if/else

Sorry I missed this earlier, but I believe Steve recommended changing these to warning instead of error: https://github.com/adobe/aepsdk-core-ios/pull/1121/files/f0d428b1e99eed5972634b0ab72c68bd8de7a801#r2017715694

Warning level here too

Should this be warning level?

nit: i think theres a typo } at the end of the log string?

nit: I know it doesn't affect what is being validated but the actual JSON file has:

"content": {
  "key1": "value1",
  "key2": "value2"
}

🙈

After going through the test cases, I believe this should be only a null check?
The spec for the rules consequence schema type only specifies that it must be a valid String

And in both iOS and Android Messaging, empty string values for both id and schema are also not interpreted as error cases:

iOS

fromRuleConsequenceEvent:

static func fromRuleConsequenceEvent(_ event: Event) -> PropositionItem? {
    guard let id = event.schemaId, let schema = event.schemaType, let schemaData = event.schemaData else {
        return nil
    }

    return PropositionItem(itemId: id, schema: schema, itemData: schemaData)
}

Event+Messaging helpers:

var schemaId: String? {
    details?[MessagingConstants.Event.Data.Key.ID] as? String
}

var schemaType: SchemaType? {
    guard let schemaString = details?[MessagingConstants.Event.Data.Key.SCHEMA] as? String else {
        return nil
    }

    return SchemaType(from: schemaString)
}

Android

fromRuleConsequenceDetail:

static PropositionItem fromRuleConsequenceDetail(final Map<String, Object> consequenceDetail) {
    PropositionItem propositionItem = null;
    try {
        final String uniqueId =
                DataReader.getString(
                        consequenceDetail, MessagingConstants.ConsequenceDetailKeys.ID);
        final SchemaType schema =
                SchemaType.fromString(
                        DataReader.getString(
                                consequenceDetail,
                                MessagingConstants.ConsequenceDetailKeys.SCHEMA));
...

I believe the spec also mentions that the detail data is allowed to be empty - should we move this validation to the specific schema handler logic (in this case under processEventHistoryOperation)?

If the implementation is updated to accept empty string for id, can we update this test to pass instead?

This consequence is missing the top level id field and I believe causes the test to fail at the consequence construction stage, not the detail -> id check stage

Can we also create a test case that covers when the detail "id" = null case?

Same thing for schema, can we create another test case that validate the "schema" = null case?

Same thing for data, can we create another test case that validate the "data" = null case?

Could we group these cases with the other missing or null property check cases? So overall the flow could be like:

• Property validation checks
  • Schema checks
  • Schema data checks
  • Schema data property checks
• insert logical checks
• insertIfNotExists logical checks
• Event History failure checks

And can we also add a case for "operation" = null?

Can we add a case for "content" = null?

Can we move this check to before the if check for insertIfNotExists? Since this requirement exists for both cases?

Also on second thought for this condition, do we also want to check that the eventCounts is also not -1? Since on database error, AndroidEventHistory getEvents sets count to -1 in that case?

I feel like it might be better to handle the database error case by not trying to record than to record?

Good catch! I've added the check as well as all the tests and comments mentioned above