Skip to content

Conversation

@marki1an
Copy link
Collaborator

@marki1an marki1an commented Jul 30, 2025

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

What's the context for the changes?

🧠 Rationale behind the change

Why did you choose to make these changes? Were there any trade-offs you had to consider?

🔎 New Bid Adapter Checklist

  • verify email contact works
  • NO fully dynamic hostnames
  • geographic host parameters are NOT required
  • direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
  • if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
  • cover an adapter configuration with an integration test

🧪 Test plan

How do you know the changes are safe to ship to production?

🏎 Quality check

  • Are your changes following our code style guidelines?
  • Are there any breaking changes in your code?
  • Does your test coverage exceed 90%?
  • Are there any erroneous console logs, debuggers or leftover code in your changes?

@marki1an marki1an self-assigned this Jul 30, 2025
@marki1an marki1an added the tests Functional or other tests label Jul 30, 2025
@marki1an marki1an changed the title Test/ruleengine Test: Rule Engine Jul 30, 2025
@marki1an marki1an added the work in progress Signals not finished work label Aug 4, 2025
@osulzhenko osulzhenko self-requested a review August 29, 2025 07:07
@osulzhenko osulzhenko added the do not merge Not the time for merging yet label Sep 8, 2025
Copy link
Collaborator

@osulzhenko osulzhenko left a comment

Choose a reason for hiding this comment

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

pls split spec into:

  • base (abstract)
  • general
  • aliases (optional)
  • validation
  • analytics

also add metrics verification:
success.no-invocation
success.noop
call
success.update

pom.xml Outdated
<configuration>
<systemPropertyVariables>
<launchContainers>false</launchContainers>
<launchContainers>true</launchContainers>
Copy link
Collaborator

Choose a reason for hiding this comment

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

roll back this change

@JsonProperty("seatnonbid")
BidRejectionReason seatNonBid = BidRejectionReason.REQUEST_BLOCKED_OPTIMIZED;

@JsonProperty("ifSyncedId")
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't change java code

import static org.prebid.server.functional.model.config.Stage.PROCESSED_AUCTION_REQUEST
import static org.prebid.server.functional.util.PBSUtils.randomString

class RuleSets {
Copy link
Collaborator

Choose a reason for hiding this comment

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

RuleSet?

List<Object> datacenters
List<Object> sources
List<Object> sids
Object pct
Copy link
Collaborator

Choose a reason for hiding this comment

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

percent?

assert !getAnalyticResults(bidResponse)

where:
requestedUfpUser << [new User(data: null), new User(data: [null]), //todo empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls remove all todo

Comment on lines 2831 to 2844
getDefaultBidRequestWithMultiplyBidders().tap {
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
site.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK
site.ext = new SiteExt(data: null)
},
getDefaultBidRequestWithMultiplyBidders().tap {
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
site.ext = new SiteExt(data: null)
},
getDefaultBidRequestWithMultiplyBidders(APP).tap {
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
app.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK
app.ext = new AppExt(data: null)
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls remove all todos

it.ruleSets[0].modelGroups[0].schema = [new RuleEngineModelSchema(function: GPP_SID_AVAILABLE)]
}

and: "Account with disabled or without rules engine"
Copy link
Collaborator

Choose a reason for hiding this comment

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

and: "Account with rules engine". Same for others

where:
distributionChannel | bidRequestClosure
SITE | { String domain ->
BidRequest.getDefaultBidRequest(distributionChannel).tap {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mb getDefaultBidRequestWithMultiplyBidders?

assert !getAnalyticResults(bidResponse)

and: "Logs should contain error"
def logs = pbsServiceWithRulesEngineModule.getLogsByTime(startTime)
Copy link
Collaborator

Choose a reason for hiding this comment

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

getLogsByText?

@osulzhenko osulzhenko marked this pull request as ready for review September 17, 2025 15:28
@osulzhenko osulzhenko removed do not merge Not the time for merging yet work in progress Signals not finished work labels Sep 17, 2025
PaaFormat paaFormat
@JsonProperty("alternatebiddercodes")
AlternateBidderCodes alternateBidderCodes
Map kvps
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JsonProperty("kvps")
Map<String, String> keyValuePairs

"adapters.${AMX}.endpoint": "$networkServiceContainer.rootUri/auction".toString()]
protected static final Map<String, String> OPENX_ALIAS_CONFIG = ["adapters.${OPENX}.aliases.${OPENX_ALIAS}.enabled" : "true",
"adapters.${OPENX}.aliases.${OPENX_ALIAS}.endpoint": "$networkServiceContainer.rootUri/auction".toString()]
protected static final String PB_RULE_ENGINE_MODULE_NAME_CODE = "pb-rule-engine"
Copy link
Collaborator

Choose a reason for hiding this comment

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

replase with ModuleName.PB_RULE_ENGINE

Comment on lines 33 to 36
protected final static String CALL_METRIC = "modules.module.%s.stage.%s.hook.%s.call"
protected final static String FAILER_METRIC = "modules.module.%s.stage.%s.hook.%s.failure"
protected final static String NOOP_METRIC = "modules.module.%s.stage.%s.hook.%s.success.noop"
protected final static String UPDATE_METRIC = "modules.module.%s.stage.%s.hook.%s.success.update"
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's always PB_RULE_ENGINE module for this spec

Stage stage
String name
String version
List<RulesEngineModelGroups> modelGroups
Copy link
Collaborator

Choose a reason for hiding this comment

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

RulesEngineModelGroup

Comment on lines 20 to 21
updateBidRequestWithGeoCountry(it)
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have 125 uses of getDefaultBidRequestWithMultiplyBidders, 36 uses of updateBidRequestWithGeoCountry & 124 - updateBidRequestWithTraceVerboseAndReturnAllBidStatus. So why not combine them?

AD_UNIT_CODE_IN("adUnitCodeIn"),
DEVICE_TYPE("deviceType"),
DEVICE_TYPE_IN("deviceTypeIn"),
BID_PRICE("bidPrice")
Copy link
Collaborator

Choose a reason for hiding this comment

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

BID_PRICE not used

Comment on lines 2795 to 2796
1 | TRUE as String
0 | FALSE as String
Copy link
Collaborator

Choose a reason for hiding this comment

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

    1    | 'TRUE'
    0    | 'FALSE'

assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE

where:
percent << [100, PBSUtils.getRandomNumber(100)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is 100% really that special? and why [100; 0x7fffffff)?

"Field 'domains' is required and has to be an array of strings")

where:
distributionChannel | bidRequestClosure
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be simplified:

bidRequestClosure <<        [     getDefaultBidRequestWithMultiplyBidders(SITE).tap {
                it.site.publisher = new Publisher(id: PBSUtils.randomString, domain: PBSUtils.randomString)
                updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
            } ]

same for others

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, why not make a methods that sets Publisher or domain based on distributionChannel? Like setAccountId. Then you will only need distributionChannel in where part

def "PBS should exclude bidder when adUnitCodeIn match with condition"() {
given: "Default bid request with multiply bidders"
def randomString = PBSUtils.randomString
def bidRequest = bidRequestClosure(randomString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

m.b. generate bidRequest before Closure and only then apply change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what if?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)

getImpUnitCode(Imp imp, ImpUnitCode) {
    switch ()
}

enum ImpUnitCode {
    TAG_ID
    GRID
    DATA
    STORED_REQUEST
}

Comment on lines 59 to 61
protected static Map<String, String> getRequestCorrectionSettings(Endpoint endpoint = OPENRTB2_AUCTION, Stage stage = PROCESSED_AUCTION_REQUEST) {
["hooks.${PB_REQUEST_CORRECTION.code}.enabled": "true",
"hooks.host-execution-plan" : encode(ExecutionPlan.getSingleEndpointExecutionPlan(endpoint, PB_REQUEST_CORRECTION, [stage]))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls restore config

Comment on lines 279 to 295
private static void updateImpWithOpenXAndAmxAndOpenXAliasBidder(Bidder bidder) {
bidder.tap {
openx = Openx.defaultOpenx
amx = new Amx()
openxAlias = Openx.defaultOpenx
}
}

private static void getAliasAndAmxAndOpenXAndWithoutGenericBidder(Bidder bidder) {
bidder.tap {
alias = new Generic()
generic = null
amx = new Amx()
openx = Openx.defaultOpenx
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

m.b.? And make it protected on RuleEngineBaseSpec for getDefaultBidRequestWithMultiplyBidders

    private static void updateBidderImp(Imp imp, List<BidderName> bidders = MULTI_BID_ADAPTERS) {
        imp.ext.prebid.bidder.tap {
            openx       = bidders.contains(OPENX)       ? Openx.defaultOpenx : null
            openxAlias  = bidders.contains(OPENX_ALIAS) ? Openx.defaultOpenx : null
            amx         = bidders.contains(AMX)         ? new Amx()          : null
            generic     = bidders.contains(GENERIC)     ? new Generic()      : null
            alias       = bidders.contains(ALIAS)       ? new Generic()      : null
        }
    }

This also allow u remove magic numbers
bidResponse.ext.seatnonbid.size() == 3/2

AD_UNIT_CODE("adUnitCode"),
AD_UNIT_CODE_IN("adUnitCodeIn"),
DEVICE_TYPE("deviceType"),
DEVICE_TYPE_IN("deviceTypeIn"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about bidPrice?

Allows response rules to be set up to trigger on bid response prices. e.g. bidPrice["gt", 5.00, "EUR"] would return true if the bid response is greater than 5 EUR.

Left as an example of possible future functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BidPrice will be later, currently it's in low priority

Stage stage
String name
String version
List<RulesEngineModelGroup> modelGroups
Copy link
Collaborator

Choose a reason for hiding this comment

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

timestamp?
ruleSets[].timestamp - an optional string indicating when this ruleset was last updated. Again - for troubleshooting. Format is ISO 8601 format.


static RulesEngineModelGroup createRulesModuleGroup() {
new RulesEngineModelGroup().tap {
it.weight = PBSUtils.getRandomNumber(0, 100)
Copy link
Collaborator

Choose a reason for hiding this comment

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

PBSUtils.getRandomNumber(1, 100)
Could be flaky:
Weight must be greater than zero

assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE

where:
distributionChannel << [SITE, APP, DOOH]
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for other
DistributionChannel.values()

assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE

where:
bidRequestClosure << [
Copy link
Collaborator

@osulzhenko osulzhenko Oct 2, 2025

Choose a reason for hiding this comment

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

    BidRequest createBidRequest(DistributionChannel type, String domain, boolean usePublisher = true) {
        def request = getDefaultBidRequestWithMultiplyBidders(type)

        switch (type) {
            case SITE:
                if (usePublisher) request.site.publisher.domain = domain
                else request.site.domain = domain
                break
            case APP:
                if (usePublisher) request.app.publisher.domain = domain
                else request.app.domain = domain
                break
            case DOOH:
                if (usePublisher) request.dooh.publisher.domain = domain
                else request.dooh.domain = domain
                break
        }
        request
    }
        type | usePublisher
        SITE | true
        SITE | false
        APP  | true
        APP  | false

assert !getAnalyticResults(bidResponse)
}

def "PBS should exclude bidder when adUnitCode match with condition"() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add priority test:
Return the first of: imp.ext.gpid, imp.tagid, imp.ext.data.pbadslot, imp.ext.prebid.storedrequest.id

In validation mode, reject args

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1.Ok
2.We have test for this condition : PBS should reject processing rule engine when #function schema function contain args

def "PBS should exclude bidder when adUnitCodeIn match with condition"() {
given: "Default bid request with multiply bidders"
def randomString = PBSUtils.randomString
def bidRequest = bidRequestClosure(randomString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)

getImpUnitCode(Imp imp, ImpUnitCode) {
    switch ()
}

enum ImpUnitCode {
    TAG_ID
    GRID
    DATA
    STORED_REQUEST
}

assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE
}

def "PBS shouldn't exclude bidder when percent not match with condition"() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when percent less than zero

Comment on lines 26 to 29
AD_UNIT_CODE("adUnitCode",null),
AD_UNIT_CODE_IN("adUnitCodeIn","codes"),
DEVICE_TYPE("deviceType",null),
DEVICE_TYPE_IN("deviceTypeIn","types"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

formating


import org.prebid.server.functional.model.request.auction.Imp

import static org.prebid.server.functional.model.ModuleName.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

replace wildcard

impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue
impResult.values.resultFunction == groups.rules.first.results.first.function.value
impResult.values.conditionFired == groups.rules.first.conditions.first
impResult.values.biddersRemoved.sort() == [OPENX, AMX, GENERIC].sort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be def value
[OPENX, AMX, GENERIC].sort()

impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue
impResult.values.resultFunction == groups.rules.first.results.first.function.value
impResult.values.conditionFired == groups.rules.first.conditions.first
impResult.values.biddersRemoved.sort() == [OPENX, GENERIC, AMX].sort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

}

protected static String getImpAdUnitCode(Imp imp) {
if (imp.ext.gpid) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add ?. for null safe

given: "Bid request with multiply imps bidders"
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
it.imp.add(Imp.defaultImpression)
it.imp[1].ext.prebid.bidder.tap {
Copy link
Collaborator

Choose a reason for hiding this comment

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

updateBidderImp?
same for others

Comment on lines 880 to 881


Copy link
Collaborator

Choose a reason for hiding this comment

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

empty space

assert !getAnalyticResults(bidResponse)

where:
gdpr | condition
Copy link
Collaborator

Choose a reason for hiding this comment

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

not consistent with previous. M.b. in lower and upper cases?

and: "Analytics result shouldn't contain info about module exclude"
assert !getAnalyticResults(bidResponse)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

empty space

Comment on lines 934 to 944
where:
gpid | tagId | pbAdSlot | prebidStoredRequest
PBSUtils.getRandomString() | null | null | null
null | PBSUtils.getRandomString() | null | null
null | null | PBSUtils.getRandomString() | null
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | PBSUtils.getRandomString() | null
null | PBSUtils.getRandomString() | null | null
PBSUtils.getRandomString() | null | null | null
Copy link
Collaborator

Choose a reason for hiding this comment

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

        where:
        gpid                       | tagId                      | pbAdSlot                   | prebidStoredRequest
        PBSUtils.getRandomString() | null                       | null                       | null
        null                       | PBSUtils.getRandomString() | null                       | null
        null                       | null                       | PBSUtils.getRandomString() | null
        null                       | null                       | null                       | new PrebidStoredRequest(id: PBSUtils.getRandomString())
        PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
        null                       | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
        null                       | null                       | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
        null                       | null                       | null                       | new PrebidStoredRequest(id: PBSUtils.getRandomString())

@osulzhenko osulzhenko merged commit b1f47a7 into ruleengine Oct 5, 2025
1 check passed
@osulzhenko osulzhenko deleted the test/ruleengine branch October 5, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Functional or other tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants