SALTO-7151: Fix "important values" generation for Swagger types #7104

Nurdok · 2025-01-14T12:03:43Z

Replace me with a description of the changes in this PR

Additional context for reviewer

Release Notes:

Fixed important values generation for Swagger types in the new infra

User Notifications:
None

coveralls · 2025-01-14T12:23:19Z

coverage: 93.649% (-0.01%) from 93.661%
when pulling 63930df on Nurdok:feature/important-values-infra
into 99be0d0 on salto-io:main.

shir-reifenberg

Looks good, added mainly questions :)

shir-reifenberg · 2025-01-14T16:30:45Z

packages/adapter-components/test/fetch/element/element.test.ts

Can you please add a test covering a case when there are predefinedTypes with important values?

shir-reifenberg · 2025-01-14T16:33:50Z

packages/adapter-components/src/fetch/element/type_utils.ts

+  log.timeDebug(() => {
+    Object.entries(definedTypes).forEach(([typeName, type]) => {
+      if (finalTypeNames?.has(typeName)) {
+        log.trace('type %s is marked as final, not adjusting', type.elemID.getFullName())


I'm not sure about this... I think in this case we should add important values anyway

shir-reifenberg · 2025-01-14T16:39:02Z

packages/adapter-components/src/fetch/element/type_utils.ts

+
+      const { element: elementDef } = defQuery.query(typeName) ?? {}
+
+      const importantValues = (elementDef?.topLevel?.importantValues ?? []).filter(


I don't understand something 🤔 If importantValues can only exist on top level types, how can we set them for fields like Group's source.id ?

(I know this definitions were there before) commenting again after I saw the okta config with the (profile.login)
something here looks odd to me... WDYT?

Only top-level types can define important values, but they can use inner values to do so. If we needed to add important values to the inner type instead, it would force all types that have a field with that inner type to consider it an important value, which is less flexible.

shir-reifenberg · 2025-01-14T16:44:55Z

packages/adapter-components/src/fetch/element/type_utils.ts

+      const { element: elementDef } = defQuery.query(typeName) ?? {}
+
+      const importantValues = (elementDef?.topLevel?.importantValues ?? []).filter(
+        ({ value }: ImportantValue) => type.fields[value] !== undefined,


just making sure .fields[value] works with dot separated path?

Replaced with getTypeInPath which I've refactored to support primitive types. Thanks for the tip!

shir-reifenberg · 2025-01-14T16:46:09Z

packages/adapter-components/src/fetch/element/type_utils.ts

+      // Avoid creating unnecessary annotations for types that don't have important values and don't need an override.
+      if (
+        _.isEmpty(importantValues) &&
+        (type.annotations === undefined || type.annotations[CORE_ANNOTATIONS.IMPORTANT_VALUES] === undefined)


why is this part of the condition needed?

Otherwise, we instantiate type.annotations or type.annotations['_important_values'] as empty objects, which creates a diff in type nacls.

shouldn't this be _.isEmpty(importantValues) || then? I'm probably missing something here, let's talk IRL 😄

Also the comment
// Avoid creating unnecessary annotations for types that don't have important values and don't need an override.
what does "don't need an override" means?

These conditions and the comment were relevant before I deleted the important value assignments in generateType - back then, some types arrived to this function with some important values that needed to be overridden based on the type's actual field.

Now that I deleted that, I've also simplified the condition and the comment. Thanks!

shir-reifenberg · 2025-01-14T16:47:11Z

packages/adapter-components/src/fetch/element/type_utils.ts

+  defQuery: ElementAndResourceDefFinder<Options>
+  finalTypeNames?: Set<string>
+}): void =>
+  log.timeDebug(() => {


nit: probably should be trace

shir-reifenberg · 2025-01-14T16:48:11Z

packages/adapter-components/src/fetch/element/type_utils.ts

+      // Generated duck-typed types may contain default important values that are not relevant,
+      // so we want to override them at this point where we know what fields are actually relevant.


Why we need to add important values attributes in the two places ?
isn't this can fully replace https://github.com/salto-io/salto/blob/main/packages/adapter-components/src/fetch/element/type_element.ts#L233 ?

shir-reifenberg

Looks great, thanks for this
let's talk IRL on https://github.com/salto-io/salto/pull/7104/files#r1921496870 cause I feel like i'm missing something
approving to unblock as the rest looks 💯

also FYI @netama incase you'd like to take a look

shir-reifenberg · 2025-01-19T16:31:27Z

packages/adapter-components/src/fetch/element/type_utils.ts

@@ -341,6 +348,64 @@ export const hideAndOmitFields = <Options extends FetchApiDefinitionsOptions>({
    })
  }, 'hideAndOmitFields')

+export const getTypeInPath = (


Thanks for adjusting & exporting this ! 🔥
As this is now exported can you please 🙏

add a docstring

cover in UT

Done™ and Done™

shir-reifenberg · 2025-01-19T16:34:16Z

packages/adapter-components/src/fetch/element/type_utils.ts

+  if (isPrimitiveType(fieldType)) {
+    log.warn('field %s in type %s is a primitive type, cannot recurse further', fieldName, type.elemID.getFullName())
+    return undefined
+  }


This case is not valid as restPath is not empty right? maybe it will be useful in the log?

Refined the log message.

shir-reifenberg · 2025-01-19T16:35:21Z

packages/adapter-components/src/fetch/element/type_utils.ts

+
+      const { element: elementDef } = defQuery.query(typeName) ?? {}
+
+      const importantValues = (elementDef?.topLevel?.importantValues ?? []).filter(


shir-reifenberg · 2025-01-19T16:50:37Z

packages/adapter-components/src/fetch/element/type_utils.ts

+      // Avoid creating unnecessary annotations for types that don't have important values and don't need an override.
+      if (
+        _.isEmpty(importantValues) &&
+        (type.annotations === undefined || type.annotations[CORE_ANNOTATIONS.IMPORTANT_VALUES] === undefined)


shouldn't this be _.isEmpty(importantValues) || then? I'm probably missing something here, let's talk IRL 😄

Also the comment
// Avoid creating unnecessary annotations for types that don't have important values and don't need an override.
what does "don't need an override" means?

shir-reifenberg

👑

Nurdok added 8 commits December 29, 2024 09:19

save

348613f

Merge branch 'main' into feature/okta-important-values

83078d7

save

21db8b2

🎨 formatting

3e38f85

Fix tests.

db41601

add test

599f6f0

🎨 formatting

ffacad6

remove okta stuff

63930df

Nurdok requested a review from shir-reifenberg January 14, 2025 12:04

Nurdok changed the title ~~SALTO-7151: Fix important values generation for Swagger types~~ SALTO-7151: Fix "important values" generation for Swagger types Jan 14, 2025

Nurdok marked this pull request as ready for review January 14, 2025 12:24

shir-reifenberg reviewed Jan 14, 2025

View reviewed changes

Nurdok force-pushed the feature/important-values-infra branch from 1baa8be to e646fe5 Compare January 19, 2025 12:52

CR fixes.

a986741

Nurdok force-pushed the feature/important-values-infra branch from e646fe5 to a986741 Compare January 19, 2025 13:00

Nurdok requested a review from shir-reifenberg January 19, 2025 13:28

shir-reifenberg approved these changes Jan 19, 2025

View reviewed changes

CR fixes.

66afc1e

shir-reifenberg approved these changes Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SALTO-7151: Fix "important values" generation for Swagger types #7104

SALTO-7151: Fix "important values" generation for Swagger types #7104

Nurdok commented Jan 14, 2025

coveralls commented Jan 14, 2025

shir-reifenberg left a comment

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 14, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 19, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 19, 2025

Nurdok Jan 20, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg Jan 14, 2025

Nurdok Jan 19, 2025

shir-reifenberg left a comment

shir-reifenberg Jan 19, 2025

Nurdok Jan 20, 2025

shir-reifenberg Jan 19, 2025

Nurdok Jan 20, 2025

shir-reifenberg Jan 19, 2025

shir-reifenberg Jan 19, 2025

shir-reifenberg left a comment


		const { element: elementDef } = defQuery.query(typeName) ?? {}

		const importantValues = (elementDef?.topLevel?.importantValues ?? []).filter(

		// Generated duck-typed types may contain default important values that are not relevant,
		// so we want to override them at this point where we know what fields are actually relevant.

SALTO-7151: Fix "important values" generation for Swagger types #7104

Are you sure you want to change the base?

SALTO-7151: Fix "important values" generation for Swagger types #7104

Conversation

Nurdok commented Jan 14, 2025

coveralls commented Jan 14, 2025

shir-reifenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shir-reifenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shir-reifenberg left a comment

Choose a reason for hiding this comment