-
Notifications
You must be signed in to change notification settings - Fork 49
Add implementation-defined maximums #332
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,10 @@ | |
| }, | ||
| "maxConversionSitesPerImpression": 3, | ||
| "maxConversionCallersPerImpression": 3, | ||
| "maxImpressionSitesForConversion": 3, | ||
| "maxImpressionCallersForConversion": 3, | ||
|
Comment on lines
5
to
+8
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know setting lower values here simplifies test construction, but it may be confusing to anyone stumbling across this code for them to be less than the minimums mandated by the specification. I'm OK with keeping them this low for now, but we might want to add a |
||
| "maxCreditSize": 10, | ||
| "maxMatchValues": 10, | ||
| "maxLookbackDays": 30, | ||
| "maxHistogramSize": 5, | ||
| "privacyBudgetMicroEpsilons": 1000000, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,8 +68,17 @@ function validateSite(input: string): void { | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| function parseSites(input: readonly string[]): Set<string> { | ||||||||||||||||||
| function parseSites( | ||||||||||||||||||
| input: readonly string[], | ||||||||||||||||||
| label: string, | ||||||||||||||||||
| limit: number, | ||||||||||||||||||
| ): Set<string> { | ||||||||||||||||||
| const parsed = new Set<string>(); | ||||||||||||||||||
| if (limit !== null && input.length > limit) { | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The |
||||||||||||||||||
| throw new RangeError( | ||||||||||||||||||
| `number of values in ${label} exceeds limit of ${limit}`, | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| for (const site of input) { | ||||||||||||||||||
| parsed.add(parseSite(site)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -80,10 +89,21 @@ export interface Delegate { | |||||||||||||||||
| readonly aggregationServices: AttributionAggregationServices; | ||||||||||||||||||
| readonly includeUnencryptedHistogram?: boolean; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The maximum number of conversion callers per impression. | ||||||||||||||||||
| readonly maxConversionSitesPerImpression: number; | ||||||||||||||||||
| /// The maximum number of conversion sites per impression. | ||||||||||||||||||
| readonly maxConversionCallersPerImpression: number; | ||||||||||||||||||
|
Comment on lines
+92
to
95
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| /// The maximum number of impression sites for conversion. | ||||||||||||||||||
| readonly maxImpressionSitesForConversion: number; | ||||||||||||||||||
| /// The maximum number of impression callers for conversion. | ||||||||||||||||||
| readonly maxImpressionCallersForConversion: number; | ||||||||||||||||||
| /// The maximum number of credit values. | ||||||||||||||||||
| readonly maxCreditSize: number; | ||||||||||||||||||
| /// The maximum number of match values. | ||||||||||||||||||
| readonly maxMatchValues: number; | ||||||||||||||||||
| /// The maximum lookback in days. | ||||||||||||||||||
| readonly maxLookbackDays: number; | ||||||||||||||||||
| /// The maximum size of histograms. | ||||||||||||||||||
| readonly maxHistogramSize: number; | ||||||||||||||||||
| readonly privacyBudgetMicroEpsilons: number; | ||||||||||||||||||
| readonly privacyBudgetEpoch: Temporal.Duration; | ||||||||||||||||||
|
|
@@ -162,23 +182,16 @@ export class Backend { | |||||||||||||||||
| } | ||||||||||||||||||
| lifetimeDays = Math.min(lifetimeDays, this.#delegate.maxLookbackDays); | ||||||||||||||||||
|
|
||||||||||||||||||
| const maxConversionSitesPerImpression = | ||||||||||||||||||
| this.#delegate.maxConversionSitesPerImpression; | ||||||||||||||||||
| if (conversionSites.length > maxConversionSitesPerImpression) { | ||||||||||||||||||
| throw new RangeError( | ||||||||||||||||||
| `conversionSites.length must be <= ${maxConversionSitesPerImpression}`, | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| const parsedConversionSites = parseSites(conversionSites); | ||||||||||||||||||
|
|
||||||||||||||||||
| const maxConversionCallersPerImpression = | ||||||||||||||||||
| this.#delegate.maxConversionCallersPerImpression; | ||||||||||||||||||
| if (conversionCallers.length > maxConversionCallersPerImpression) { | ||||||||||||||||||
| throw new RangeError( | ||||||||||||||||||
| `conversionCallers.length must be <= ${maxConversionCallersPerImpression}`, | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| const parsedConversionCallers = parseSites(conversionCallers); | ||||||||||||||||||
| const parsedConversionSites = parseSites( | ||||||||||||||||||
| conversionSites, | ||||||||||||||||||
| "conversionSites", | ||||||||||||||||||
| this.#delegate.maxConversionSitesPerImpression, | ||||||||||||||||||
| ); | ||||||||||||||||||
| const parsedConversionCallers = parseSites( | ||||||||||||||||||
| conversionCallers, | ||||||||||||||||||
| "conversionCallers", | ||||||||||||||||||
| this.#delegate.maxConversionCallersPerImpression, | ||||||||||||||||||
| ); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (matchValue < 0 || !Number.isInteger(matchValue)) { | ||||||||||||||||||
| throw new RangeError("matchValue must be a non-negative integer"); | ||||||||||||||||||
|
|
@@ -270,21 +283,38 @@ export class Backend { | |||||||||||||||||
| lookbackDays = Math.min(lookbackDays, this.#delegate.maxLookbackDays); | ||||||||||||||||||
|
|
||||||||||||||||||
| const matchValueSet = new Set<number>(); | ||||||||||||||||||
| const maxMatchValues = this.#delegate.maxMatchValues; | ||||||||||||||||||
| if (matchValues.length > maxMatchValues) { | ||||||||||||||||||
| throw new RangeError( | ||||||||||||||||||
| `matchValues size must be in the range [0,${maxMatchValues}]`, | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
To match the error format for |
||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| for (const value of matchValues) { | ||||||||||||||||||
| if (value < 0 || !Number.isInteger(value)) { | ||||||||||||||||||
| throw new RangeError("match value must be a non-negative integer"); | ||||||||||||||||||
| } | ||||||||||||||||||
| matchValueSet.add(value); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const parsedImpressionSites = parseSites( | ||||||||||||||||||
| impressionSites, | ||||||||||||||||||
| "impressionSites", | ||||||||||||||||||
| this.#delegate.maxImpressionSitesForConversion, | ||||||||||||||||||
| ); | ||||||||||||||||||
| const parsedImpressionCallers = parseSites( | ||||||||||||||||||
| impressionCallers, | ||||||||||||||||||
| "impressionCallers", | ||||||||||||||||||
| this.#delegate.maxImpressionCallersForConversion, | ||||||||||||||||||
| ); | ||||||||||||||||||
|
|
||||||||||||||||||
| return { | ||||||||||||||||||
| aggregationService: aggregationServiceEntry, | ||||||||||||||||||
| epsilon, | ||||||||||||||||||
| histogramSize, | ||||||||||||||||||
| lookback: days(lookbackDays), | ||||||||||||||||||
| matchValues: matchValueSet, | ||||||||||||||||||
| impressionSites: parseSites(impressionSites), | ||||||||||||||||||
| impressionCallers: parseSites(impressionCallers), | ||||||||||||||||||
| impressionSites: parsedImpressionSites, | ||||||||||||||||||
| impressionCallers: parsedImpressionCallers, | ||||||||||||||||||
| credit, | ||||||||||||||||||
| value, | ||||||||||||||||||
| maxValue, | ||||||||||||||||||
|
|
@@ -661,7 +691,7 @@ export class Backend { | |||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| clearState(sites: readonly string[], forgetVisits: boolean): void { | ||||||||||||||||||
| const parsedSites = parseSites(sites); | ||||||||||||||||||
| const parsedSites = parseSites(sites, "sites", Infinity); | ||||||||||||||||||
| if (!forgetVisits) { | ||||||||||||||||||
| this.#zeroBudgetForSites(parsedSites); | ||||||||||||||||||
| return; | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to update
impl/e2e.schema.jsonas well with the new configuration fields.