Internal Bugfix - add telemetry guards for string overflow #4538
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package software.aws.toolkits.jetbrains.services.codemodernizer.model | ||
|
|
||
| import com.fasterxml.jackson.databind.module.SimpleModule | ||
| import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper | ||
|
|
||
| const val CODETRANSFORM_METADATA_MAX_STRINGIFIED_LENGTH = 65536 | ||
|
|
||
| data class CodeTransformTelemetryMetadata( | ||
| var dependencyVersionSelected: String? = null, | ||
| var cancelledFromChat: Boolean = false, | ||
| ) { | ||
| private val propertyValues = listOf( | ||
| "dependencyVersionSelected" to dependencyVersionSelected, | ||
| "cancelledFromChat" to cancelledFromChat | ||
| ) | ||
|
|
||
| operator fun iterator(): Iterator<Pair<String, Any?>> = propertyValues.iterator() | ||
|
|
||
| fun toJsonString(): String { | ||
| var trimmedJsonString = trimJsonString(CODETRANSFORM_METADATA_MAX_STRINGIFIED_LENGTH) | ||
| return trimmedJsonString | ||
| } | ||
|
|
||
| fun resetDefaults() { | ||
| dependencyVersionSelected = null | ||
| cancelledFromChat = false | ||
| } | ||
|
|
||
| /** | ||
| * @description We have a truncation function for all fields to be less than 1000 characters. | ||
| * If this fails, we try to completely remove fields to limit the size sent to backend to prevent | ||
| * an overflow when submitting data. | ||
| */ | ||
| private fun trimJsonString(maxLength: Int): String { | ||
| val objectMapper = jacksonObjectMapper() | ||
| objectMapper.registerModule( | ||
| SimpleModule().addSerializer(String::class.java, MaxLengthTelemetryStringSerializer()) | ||
| ) | ||
| val jsonString = objectMapper.writeValueAsString(this) | ||
| if (jsonString.length <= maxLength) { | ||
| return jsonString | ||
| } | ||
|
|
||
| val trimmedPropertyValues = mutableListOf<Pair<String, Any?>>() | ||
| var currentLength = 0 | ||
| for ((key, value) in propertyValues) { | ||
| val elementLength = key.length + value.toString().length + 5 // add 5 for quotes and comma around key-value pairs | ||
| if (currentLength + elementLength <= maxLength) { | ||
| trimmedPropertyValues.add(Pair(key, value)) | ||
| currentLength += elementLength | ||
| } | ||
| // else we omit the key/value pair as a way of "trimming" the object that is too large | ||
| } | ||
|
|
||
| return objectMapper.writeValueAsString(trimmedPropertyValues.toMap()) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package software.aws.toolkits.jetbrains.services.codemodernizer.model | ||
|
|
||
| object CodeTransformTelemetryMetadataSingleton { | ||
|
There was a problem hiding this comment. i like the modularization for testing, but now your project service is sharing state across all instances, which is probably not what you want There was a problem hiding this comment. We only have one instance at a time right now and we do want to share this across any reference to it. For simplicity we are not managing the state of the object within any of our manager classes. If we move to a multi instance service, we can make this a variable with session or sessionContext. There was a problem hiding this comment. in the interest of avoiding global state that isn't managed by the platform then, can you convert this into a light service? your test will need to have an There was a problem hiding this comment. Sure. I'll check out adding these changes to convert to: |
||
| private val instance = CodeTransformTelemetryMetadata() | ||
|
|
||
| fun getInstance() = instance | ||
|
There was a problem hiding this comment. getInstance() in this project implies the platform "service" construct, rather than a singleton There was a problem hiding this comment. Are you saying the coding style for this repo of If its service level, then it would fit because this is a single instance shared across our service preventing duplicates. There was a problem hiding this comment. service as in the intellij platform service construct |
||
|
|
||
| fun setDependencyVersionSelected(version: String?) { | ||
| instance.dependencyVersionSelected = version | ||
| } | ||
|
|
||
| fun setCancelledFromChat(cancelled: Boolean) { | ||
| instance.cancelledFromChat = cancelled | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| package software.aws.toolkits.jetbrains.services.codemodernizer.model | ||
|
|
||
| import com.fasterxml.jackson.core.JsonGenerator | ||
| import com.fasterxml.jackson.databind.JsonSerializer | ||
| import com.fasterxml.jackson.databind.SerializerProvider | ||
|
|
||
| const val MAX_SERIALIZABLE_STRING_LENGTH = 1000 | ||
| class MaxLengthTelemetryStringSerializer : JsonSerializer<String>() { | ||
| override fun serialize(value: String, gen: JsonGenerator, provider: SerializerProvider) { | ||
| val truncatedValue = if (value.length > MAX_SERIALIZABLE_STRING_LENGTH) { | ||
| value.substring(0, MAX_SERIALIZABLE_STRING_LENGTH) | ||
| } else { | ||
| value | ||
| } | ||
| gen.writeString(truncatedValue) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| package software.aws.toolkits.jetbrains.services.codemodernizer.model | ||
|
|
||
| import junit.framework.TestCase.assertEquals | ||
| import org.junit.Test | ||
|
|
||
| open class CodeTransformTelemetryMetadataSingletonTest { | ||
|
|
||
| @Test | ||
| fun `CodeTransformTelemetryMetadata will set values and resets defaults properly`() { | ||
| CodeTransformTelemetryMetadataSingleton.setDependencyVersionSelected("1.2.3") | ||
| CodeTransformTelemetryMetadataSingleton.setCancelledFromChat(true) | ||
| assertEquals(CodeTransformTelemetryMetadataSingleton.getInstance().dependencyVersionSelected, "1.2.3") | ||
| assertEquals(CodeTransformTelemetryMetadataSingleton.getInstance().cancelledFromChat, true) | ||
|
|
||
| // check reset defaults works | ||
| CodeTransformTelemetryMetadataSingleton.getInstance().resetDefaults() | ||
| assertEquals(CodeTransformTelemetryMetadataSingleton.getInstance().dependencyVersionSelected, null) | ||
| assertEquals(CodeTransformTelemetryMetadataSingleton.getInstance().cancelledFromChat, false) | ||
| } | ||
|
|
||
| @Test | ||
| fun `CodeTransformTelemetryMetadataSingletonTest toJsonString() will serialize object correctly`() { | ||
| CodeTransformTelemetryMetadataSingleton.setDependencyVersionSelected("1.2.3") | ||
| CodeTransformTelemetryMetadataSingleton.setCancelledFromChat(true) | ||
| val expectedJsonString = """{"dependencyVersionSelected":"1.2.3","cancelledFromChat":true}""" | ||
| assertEquals(expectedJsonString, CodeTransformTelemetryMetadataSingleton.getInstance().toJsonString()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `CodeTransformTelemetryMetadataSingletonTest trimJsonString() trims single field JSON string to specified length`() { | ||
| val longString = "a".repeat(CODETRANSFORM_METADATA_MAX_STRINGIFIED_LENGTH) | ||
| CodeTransformTelemetryMetadataSingleton.setDependencyVersionSelected(longString) | ||
| CodeTransformTelemetryMetadataSingleton.setCancelledFromChat(true) | ||
|
|
||
| val expectedTrimmedJsonString = """{"dependencyVersionSelected":"${"a".repeat(MAX_SERIALIZABLE_STRING_LENGTH)}","cancelledFromChat":true}""" | ||
| assertEquals(expectedTrimmedJsonString, CodeTransformTelemetryMetadataSingleton.getInstance().toJsonString()) | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
don't need to fix, but in future avoid unnecessary diffs