Skip to content
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

✨ Allow to modify source_url property in Loaf script attributions #3325

Merged
merged 7 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/rum-core/src/domain/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ export function startRumAssembly(
...ROOT_MODIFIABLE_FIELD_PATHS,
},
[RumEventType.LONG_TASK]: {
'long_task.scripts[].source_url': 'string',
'long_task.scripts[].invoker': 'string',
...USER_CUSTOMIZABLE_FIELD_PATHS,
...VIEW_MODIFIABLE_FIELD_PATHS,
},
Expand Down
33 changes: 27 additions & 6 deletions packages/rum-core/src/domain/limitModification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,57 @@ import type { ModifiableFieldPaths } from './limitModification'
import { limitModification } from './limitModification'

describe('limitModification', () => {
let object: unknown

beforeEach(() => {
object = {
foo: { bar: 'bar' },
arr: [{ foo: 'foo' }],
qux: 'qux',
}
})

it('should allow modifications on modifiable field', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
candidate.foo.bar = 'modified1'
candidate.qux = 'modified2'
candidate.arr[0].foo = 'modified3'
}

limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)
limitModification(
object,
{
'foo.bar': 'string',
qux: 'string',
'arr[].foo': 'string',
},
modifier
)

expect(object).toEqual({
foo: { bar: 'modified1' },
qux: 'modified2',
arr: [{ foo: 'modified3' }],
})
})

it('should not allow modifications on non modifiable field', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
candidate.foo.bar = 'modified1'
candidate.qux = 'modified2'
candidate.arr[0].foo = 'modified3'
}

limitModification(object, { 'foo.bar': 'string' }, modifier)

expect(object).toEqual({
foo: { bar: 'modified1' },
arr: [{ foo: 'foo' }],
qux: 'qux',
})
})

it('should allow to add a modifiable fields not present on the original object', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
candidate.foo.bar = 'modified1'
candidate.qux = 'modified2'
Expand All @@ -46,13 +65,13 @@ describe('limitModification', () => {

expect(object as any).toEqual({
foo: { bar: 'modified1' },
arr: [{ foo: 'foo' }],
qux: 'modified2',
qix: 'modified3',
})
})

it('should not allow to add a non modifiable fields not present on the original object', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
candidate.foo.bar = 'modified1'
candidate.qux = 'modified2'
Expand All @@ -63,6 +82,7 @@ describe('limitModification', () => {

expect(object).toEqual({
foo: { bar: 'modified1' },
arr: [{ foo: 'foo' }],
qux: 'modified2',
})
})
Expand Down Expand Up @@ -119,18 +139,19 @@ describe('limitModification', () => {
})

it('should not allow structural change of the object', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
candidate.foo.bar = { qux: 'qux' }
candidate.bar = 'bar'
delete candidate.qux
;(candidate.arr as Array<Record<string, string>>).push({ bar: 'baz' })
}

limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)

expect(object).toEqual({
foo: { bar: 'bar' },
qux: 'qux',
arr: [{ foo: 'foo' }],
})
})

Expand Down
109 changes: 85 additions & 24 deletions packages/rum-core/src/domain/limitModification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import type { Context } from '@datadog/browser-core'
export type ModifiableFieldPaths = Record<string, 'string' | 'object'>

/**
* Current limitation:
* - field path do not support array, 'a.b.c' only
* Allows declaring and enforcing modifications to specific fields of an object.
* Only supports modifying properties of an object (even if nested in an array).
* Does not support array manipulation (adding/removing items).
*/
export function limitModification<T extends Context, Result>(
object: T,
Expand All @@ -14,43 +15,79 @@ export function limitModification<T extends Context, Result>(
): Result | undefined {
const clone = deepClone(object)
const result = modifier(clone)

// Validate the modified fields and assign them back to 'original' if they match the expected type.
objectEntries(modifiableFieldPaths).forEach(([fieldPath, fieldType]) => {
const newValue = get(clone, fieldPath)
const newType = getType(newValue)
if (newType === fieldType) {
set(object, fieldPath, sanitize(newValue))
const pathSegments = fieldPath.split(/\.|(?=\[\])/)
const newValue = getValueFromPath(clone, pathSegments)
const newType = getTypes(newValue)

if (isValidType(newType, fieldType)) {
setValueAtPath(object, pathSegments, sanitizeValues(newValue))
} else if (fieldType === 'object' && (newType === 'undefined' || newType === 'null')) {
set(object, fieldPath, {})
setValueAtPath(object, pathSegments, {})
}
})

return result
}

function get(object: unknown, path: string) {
let current = object
for (const field of path.split('.')) {
if (!isValidObjectContaining(current, field)) {
function getValueFromPath(object: unknown, pathSegments: string[]): unknown {
const [field, ...restPathSegments] = pathSegments

if (field === '[]') {
if (!Array.isArray(object)) {
return
}
current = current[field]

return object.map((item) => getValueFromPath(item, restPathSegments))
}

return getNestedValue(object, pathSegments)
}

function getNestedValue(object: unknown, pathSegments: string[]): unknown {
const [field, ...restPathSegments] = pathSegments

if (!isValidObjectContaining(object, field)) {
return
}

if (restPathSegments.length === 0) {
return object[field]
}
return current

return getValueFromPath(object[field], restPathSegments)
}

function set(object: unknown, path: string, value: unknown) {
let current = object
const fields = path.split('.')
for (let i = 0; i < fields.length; i += 1) {
const field = fields[i]
if (!isValidObject(current)) {
function setValueAtPath(object: unknown, pathSegments: string[], value: unknown) {
const [field, ...restPathSegments] = pathSegments

if (field === '[]') {
if (!Array.isArray(object) || !Array.isArray(value)) {
return
}
if (i !== fields.length - 1) {
current = current[field]
} else {
current[field] = value
}

object.forEach((item, i) => setValueAtPath(item, restPathSegments, value[i]))
return
}

setNestedValue(object, pathSegments, value)
}

function setNestedValue(object: unknown, pathSegments: string[], value: unknown) {
const [field, ...restPathSegments] = pathSegments

if (!isValidObject(object)) {
return
}

if (restPathSegments.length === 0) {
object[field] = value
return
}

setValueAtPath(object[field], restPathSegments, value)
}

function isValidObject(object: unknown): object is Record<string, unknown> {
Expand All @@ -60,3 +97,27 @@ function isValidObject(object: unknown): object is Record<string, unknown> {
function isValidObjectContaining(object: unknown, field: string): object is Record<string, unknown> {
return isValidObject(object) && Object.prototype.hasOwnProperty.call(object, field)
}

function isValidType(actualType: string | string[], expectedType: 'string' | 'object'): boolean {
if (Array.isArray(actualType)) {
return actualType.length > 0 && actualType.every((type) => type === expectedType)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't validate here that the length matches; is that expected? I may be misreading the code, but it seems like this will allow people to remove items from the array while still passing the isValidType() check; the result looks like it would be that you'd get undefined for some elements in the output array. That may be OK, but I just wanted to make sure it was intentional. (Or that it's handled somehow and I'm just missing it.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I haven't consider this. Actually, it also means that every property of the clone is a valid type. If only some are, none will be copied to the original object.

I've come up with a better algorithm, that should be resilient to such inconsistencies in the clone: traverse both, the object and the clone recursively at the same time, up to the end of the path where I can check the path individually.

At the end, I think the code is simpler and even more efficient as we now do the get and set traversal at once

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I really like the final result! Nicely done.

}

return actualType === expectedType
}

function sanitizeValues(thing: unknown) {
if (Array.isArray(thing)) {
return thing.map((item) => sanitize(item))
}

return sanitize(thing)
}

function getTypes(thing: unknown) {
if (Array.isArray(thing)) {
return thing.map((item) => getType(item))
}

return getType(thing)
}