Implement `mostRecent` search type for rules historical condition by timkimadobe · Pull Request #1138 · adobe/aepsdk-core-ios

timkimadobe · 2025-04-29T03:20:24Z

Description

This PR implements the mostRecent search type for rules historical condition, to support the requalification workflow.

The mostRecent search type is defined as follows:

The return type for this search is an integer. It represents the 0-based index of the most recent event, or -1 if none of the events are present in the event history.

The formats of the events parameter and its anonymous objects are the same as currently defined for historical conditions.

For example, given three distinct events in the events parameter: A, B, and C:

events: [A, B, C]

The system searches for all three events in the event history and checks which event is the most recent. For example, if B is the most recent event, the return value is 1. If A is the most recent event, the return value is 0.

A new Operand function called getMostRecentHistoricalEvent has been created to perform the most recent search, which requires as inputs:

runtime: ExtensionRuntime - to call getHistoricalEvents
requestEvents: [EventHistoryRequest] - to know which events to compare to return most recent

For each event provided in the condition, it searches event history and returns the index of the event with the newest occurrence, or -1 if none exist or a database error was encountered.

This early exit and return -1 is adopted from the existing getHistoricalEventCount implementation

Also adds new warnings for early exit or fallback paths in the related extractHistoricalCondition method

Alternative implementations

Considered potentially concurrently dispatching the getHistoricalEvents requests. Doesn't make sense for two reasons:

First and foremost all database requests go through EventHistoryDatabase's serial queue, which means even if requests arrive concurrently, they are processed synchronously from the perspective of the database
Even if the first point wasn't the case, it is an explosion of complexity for very little gain - currently the most complex use case for this logic is 2 events at most (qualify, unqualify)

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

timkimadobe · 2025-04-30T00:50:30Z

AEPCore/Sources/rules/JSONRulesParser.swift

        let semaphore = DispatchSemaphore(value: 0)

        runtime.getHistoricalEvents(requestEvents, enforceOrder: searchType == .ordered) { results in
+            defer { semaphore.signal() }


This is unrelated to the mostRecent support, but I refactored the individual semaphore signals to use a defer instead in getHistoricalEventCount

Wondering if we should avoid making changes to what's not broken? (same question for the new params.count >= 3 to avoid index out of bounds errors (although based on the current code should be impossible))

i'm with you on the "don't fix what's not broken" change. thoughts on both suggestions though:

the defer looks like it's in the right spot and shouldn't change any functionality (our tests should confirm this!)

i'm totally fine with a param count check on 205 - it doesn't change anything functionally but could serve as a fast fail if we don't have at least those 3 parameters we need.

similar to the comment made earlier in the review, i'm thinking that by using a switch statement inside of this callback we'd add some clarity to someone reading the code. what do you think about the idea of a little refactor?

Doing the switch refactor really drove home the point that .mostRecent was being silently handled by the else case in the existing logic...

For this refactor, I made the following changes:

Added an early exit for the unsupported .mostRecent type to return the error value of -1 (since 0 is a valid and specific meaning for that search type)

Converted the nested if/else's into a switch handling all cases without a default

No default, so that any future cases will cause a compiler error and need to be explicitly handled (much unlike how adding .mostRecent had unnoticed potential side effects (although impossible with the current logic)

It does require a bit of redundancy to include the mostRecent here in the switch cases too, but I thought that the benefit of the compiler error point above outweighed this

The database error check has been turned into a guard instead

Please let me know what you think!

sbenedicadb · 2025-04-30T21:46:33Z

AEPCore/Sources/rules/JSONRulesParser.swift

+    init?(_ optionalValue: String?) {
+        guard let value = optionalValue, let enumValue = EventHistorySearchType(rawValue: value) else {
+            return nil
+        }
+        self = enumValue
+    }


nit: talk me into this.

it feels unnecessary, as the rawValue initializer is effectively doing the same thing

As we discussed offline:

Removed this custom init which was basically adding a lot of unnecessary infrastructure just to handle unwrapping a String

Updated the search type to be even Swiftier 🤩 by using flatMap on the definition’s searchType, defaulting to .any when the value is either missing or fails to initialize a valid EventHistorySearchType

sbenedicadb · 2025-04-30T21:52:53Z

AEPCore/Sources/rules/JSONRulesParser.swift

+        let historyOperand: Operand<Int>
+        if searchType == .mostRecent {
+            historyOperand = Operand<Int>(function: getMostRecentHistoricalEvent, parameters: params)
+        } else {
+            // .any or .ordered
+            historyOperand = Operand<Int>(function: getHistoricalEventCount, parameters: params)
+        }


we're handling 3 different kinds of search types at this point - a more robust solution here would be to use a switch statement. it also makes it more clear to the reader (via code) how each case is handled, rather than depending on comments

Thanks, thats a great point - updated to switch instead

sbenedicadb · 2025-04-30T22:04:35Z

AEPCore/Sources/rules/JSONRulesParser.swift

        let semaphore = DispatchSemaphore(value: 0)

        runtime.getHistoricalEvents(requestEvents, enforceOrder: searchType == .ordered) { results in
+            defer { semaphore.signal() }


i'm with you on the "don't fix what's not broken" change. thoughts on both suggestions though:

the defer looks like it's in the right spot and shouldn't change any functionality (our tests should confirm this!)

i'm totally fine with a param count check on 205 - it doesn't change anything functionally but could serve as a fast fail if we don't have at least those 3 parameters we need.

similar to the comment made earlier in the review, i'm thinking that by using a switch statement inside of this callback we'd add some clarity to someone reading the code. what do you think about the idea of a little refactor?

sbenedicadb · 2025-04-30T22:06:58Z

AEPCore/Sources/rules/JSONRulesParser.swift

        return returnValue
    }

+    /// Returns the index of the most recent historical event based on occurrence date.


nit: date > timestamp

Thank you! Updated

sbenedicadb · 2025-04-30T22:17:40Z

AEPCore/Sources/rules/JSONRulesParser.swift

+        runtime.getHistoricalEvents(requestEvents, enforceOrder: false) { eventResults in
+            defer { semaphore.signal() }
+
+            for (index, result) in eventResults.enumerated() {


clever 🤩

…emove default value logging Replace search type operand handling with switch Refactor getHistoricalEventCount with switch instead of nested if else and add early exit guard for unsupported type Fix doc error

timkimadobe

Thanks so much for the review @sbenedicadb! Updated based on feedback

timkimadobe · 2025-04-30T22:57:17Z

AEPCore/Sources/rules/JSONRulesParser.swift

+    init?(_ optionalValue: String?) {
+        guard let value = optionalValue, let enumValue = EventHistorySearchType(rawValue: value) else {
+            return nil
+        }
+        self = enumValue
+    }


As we discussed offline:

Removed this custom init which was basically adding a lot of unnecessary infrastructure just to handle unwrapping a String

Updated the search type to be even Swiftier 🤩 by using flatMap on the definition’s searchType, defaulting to .any when the value is either missing or fails to initialize a valid EventHistorySearchType

timkimadobe · 2025-04-30T22:59:04Z

AEPCore/Sources/rules/JSONRulesParser.swift

+        let historyOperand: Operand<Int>
+        if searchType == .mostRecent {
+            historyOperand = Operand<Int>(function: getMostRecentHistoricalEvent, parameters: params)
+        } else {
+            // .any or .ordered
+            historyOperand = Operand<Int>(function: getHistoricalEventCount, parameters: params)
+        }


Thanks, thats a great point - updated to switch instead

timkimadobe · 2025-04-30T23:46:35Z

AEPCore/Sources/rules/JSONRulesParser.swift

        let semaphore = DispatchSemaphore(value: 0)

        runtime.getHistoricalEvents(requestEvents, enforceOrder: searchType == .ordered) { results in
+            defer { semaphore.signal() }


Doing the switch refactor really drove home the point that .mostRecent was being silently handled by the else case in the existing logic...

For this refactor, I made the following changes:

Added an early exit for the unsupported .mostRecent type to return the error value of -1 (since 0 is a valid and specific meaning for that search type)

Converted the nested if/else's into a switch handling all cases without a default

No default, so that any future cases will cause a compiler error and need to be explicitly handled (much unlike how adding .mostRecent had unnoticed potential side effects (although impossible with the current logic)

It does require a bit of redundancy to include the mostRecent here in the switch cases too, but I thought that the benefit of the compiler error point above outweighed this

The database error check has been turned into a guard instead

Please let me know what you think!

timkimadobe · 2025-04-30T23:47:24Z

AEPCore/Sources/rules/JSONRulesParser.swift

        return returnValue
    }

+    /// Returns the index of the most recent historical event based on occurrence date.


Thank you! Updated

timkimadobe · 2025-05-01T00:08:51Z

AEPCore/Sources/rules/JSONRulesParser.swift

              let runtime = params[0] as? ExtensionRuntime,
              let requestEvents = params[1] as? [EventHistoryRequest],
              let searchType = params[2] as? EventHistorySearchType else {
            return 0


Also why does missing input values return 0? Isn't this an error case? Should getMostRecentHistoricalEvent not be returning -1 like it currently is in it's equivalent validation?

AEPCore/Sources/rules/JSONRulesParser.swift

timkimadobe requested review from sbenedicadb and spoorthipujariadobe April 29, 2025 03:20

timkimadobe added 2 commits April 29, 2025 17:47

Implement the mostRecent search type handler

8717d65

Fix logical error in description

5fed783

timkimadobe force-pushed the requalification-support branch from 8c9fc24 to 5fed783 Compare April 30, 2025 00:47

timkimadobe commented Apr 30, 2025

View reviewed changes

timkimadobe mentioned this pull request Apr 30, 2025

Test coverage for JSONCondition getMostRecentHistoricalEvent (search type mostRecent) #1140

Merged

10 tasks

sbenedicadb requested changes Apr 30, 2025

View reviewed changes

Update search type logic to handle setting value more elegantly and r…

c55c8ed

…emove default value logging Replace search type operand handling with switch Refactor getHistoricalEventCount with switch instead of nested if else and add early exit guard for unsupported type Fix doc error

timkimadobe commented Apr 30, 2025

View reviewed changes

timkimadobe commented May 1, 2025

View reviewed changes

timkimadobe mentioned this pull request May 1, 2025

Test coverage for JSONCondition getHistoricalEventCount (search type any or ordered) #1142

Merged

10 tasks

spoorthipujariadobe reviewed May 6, 2025

View reviewed changes

AEPCore/Sources/rules/JSONRulesParser.swift Show resolved Hide resolved

timkimadobe requested a review from sbenedicadb May 6, 2025 18:32

spoorthipujariadobe approved these changes May 28, 2025

View reviewed changes

sbenedicadb approved these changes May 28, 2025

View reviewed changes

timkimadobe merged commit 87934f4 into adobe:feature/historical-consequence May 28, 2025
17 checks passed

timkimadobe deleted the requalification-support branch May 28, 2025 23:36

This was referenced Jul 24, 2025

Support for new historical rules condition and consequences #1161

Closed

Support new historical rules condition and consequences #1162

Merged

Conversation

timkimadobe commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Alternative implementations

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timkimadobe commented Apr 29, 2025 •

edited

Loading