Generator Should Read ExternalImportType Safety #1944

lsingh123 · 2023-02-01T23:33:43Z

If an Alias, Field, or Argument is an external import type, use the underlying import's safety instead of the declared safety (which will be empty). Converts all usages of AliasDefinition.getSafety(), FieldDefinition.getSafety(), and ArgumentDefinition.getSafety() to the corresponding wrapper methods in SafetyUtils.java.

lsingh123 · 2023-02-02T21:34:09Z

#1945 is a better solution

…imports-log-safety

conjure-java-core/src/integrationInput/java/com/palantir/product/SafeExternalLongExample.java

lsingh123 · 2023-02-09T19:20:39Z

conjure-java-core/src/integrationInput/java/com/palantir/product/SafeExternalLongExample.java

+        }
+
+        @JsonSetter(value = "optionalSafeExternalLong", nulls = Nulls.SKIP)
+        public Builder optionalSafeExternalLong(@Nonnull Optional<? extends Long> optionalSafeExternalLong) {


again, not sure if we want the parameter to have a safety annotation, or if we don't support wrapping external references in optional

i think i am actually going to handle optionals wrapping externals, pushing that up soon

Ideally I think we'd end up with Optional<@Safe ? extends Long>

Would we want @Safe Optional<@Safe ? extends Long> like here? Or would we omit the external annotation for external imports?

I think the linked example should ideally only have a single @Safe:

public Builder item(@Nonnull Optional<@Safe String> item) {

For posterity:

Poet has a bug that will make handling containers of external imports hard (like optional). We use WildcardTypeName to represent the wildcard type extending an external import. But when emitting, WildcardTypeName essentially ignores its annotations, which I verified in this (extremely rough) test. So in order to generate code that looks like @Safe ? extends Long> we would have to do something extremely hacky, like eschew javapoet and just stick in a string which doesn't feel......great.

So we decided to settle for the annotation on the wrapper itself, as in @Safe Optional<...>

conjure-java-core/src/main/java/com/palantir/conjure/java/util/SafetyUtils.java

lsingh123 · 2023-02-10T21:38:16Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java

        FieldSpec field = enriched.poetSpec();
-        return BeanBuilderAuxiliarySettersUtils.createItemSetterBuilder(enriched, itemType, typeMapper, builderClass)
+        return BeanBuilderAuxiliarySettersUtils.createItemSetterBuilder(


previously, item setters for collection types didn't have safety annotations, even for primitive collections. For example, a field of type List<boolean> would generate the following setter:

public Builder listBoolean(String listBoolean) { checkNotBuilt(); this.listBoolean.add(listBoolean); return this; }

Now, both primitive and external import collections generate item setters that look like:

public Builder listBoolean(@Safe String listBoolean) { checkNotBuilt(); this.listBoolean.add(listBoolean); return this; }

lsingh123 · 2023-02-10T21:39:23Z

conjure-java-core/src/test/java/com/palantir/conjure/java/types/ExternalImportSafetyTests.java

+
+        Class<?> builderSubclass = getMatchingSubclass(SafeExternalLongExample.class, "$Builder");
+        assertFieldTypeParamHasAnnotation(builderSubclass, "safeExternalLongSet", "Long", Safe.class);
+        assertMethodParamHasAnnotation(builderSubclass, "safeExternalLongSet", "safeExternalLongSet", Safe.class);


this includes the item setter and the collection setter because the internal method uses allMatch

lsingh123 · 2023-02-13T19:01:02Z

...ava-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java

        FieldSpec field = enriched.poetSpec();
-        return publicSetter(enriched, returnClass).addParameter(typeMapper.getClassName(itemType), field.name);
+        return publicSetter(enriched, returnClass)


this is to support this https://github.com/palantir/conjure-java/pull/1944/files#r1103276431

changelog-app · 2023-02-13T19:03:12Z

Generate changelog in `changelog/@unreleased`

Type

Description

Adds support for log safety annotations to external import types. For example, an alias of an external reference type will now have safety annotations on the class, getters, and setters.

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-02-14T16:16:31Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java

@@ -113,7 +115,7 @@ public Optional<LogSafety> visitUnion(UnionDefinition value) {
            return with(value.getTypeName(), () -> {
                Optional<LogSafety> safety = UNKNOWN_UNION_VARINT_SAFETY;
                for (FieldDefinition variant : value.getUnion()) {
-                    safety = combine(safety, getSafety(variant.getType(), variant.getSafety()));
+                    safety = combine(safety, getSafety(variant.getType(), SafetyUtils.getMaybeExternalSafety(variant)));


If I understand this correctly, I don't think any of the SafetyUtils utility-method invocations used in this class are necessary. This class will walk the tree until it finds something that declares safety, so the updated visitExternal(ExternalReference value) should take care of it.

carterkozak · 2023-02-14T16:56:18Z

conjure-java-core/src/main/java/com/palantir/conjure/java/util/SafetyUtils.java

+    public static Optional<LogSafety> getMaybeExternalSafety(AliasDefinition alias) {
+        if (alias.getAlias().accept(WrapsExternalType.INSTANCE)) {
+            return alias.getAlias().accept(GetMaybeExternalSafety.INSTANCE);
+        }
+        return alias.getSafety();
+    }
+
+    public static Optional<LogSafety> getMaybeExternalSafety(FieldDefinition field) {
+        if (field.getType().accept(WrapsExternalType.INSTANCE)) {
+            return field.getType().accept(GetMaybeExternalSafety.INSTANCE);
+        }
+        return field.getSafety();
+    }
+
+    public static Optional<LogSafety> getMaybeExternalSafety(ArgumentDefinition argument) {
+        if (argument.getType().accept(WrapsExternalType.INSTANCE)) {
+            return argument.getType().accept(GetMaybeExternalSafety.INSTANCE);
+        }
+        return argument.getSafety();
+    }


I might reframe and refactor these a bit to decouple behavior from external types. There's a class of values (applies to fields, arguments, and aliases) which require their safety to be explicitly listed. So, given a Type, we need to determine whether the generated class declares safety itself, or if we need to add an annotation where the type is used.

Primitives (with the exception of bearer-token) and external-type-imports require safety at each place they're used. Determining the safety value can be handled using the existing SafetyEvaluator class.

Ah, so I could instead think of this class as getting the usage time safety, which, for primitives, is declared in the wrapper type (field, argument, alias) itself and for external imports, is declared in the external-import-type itself.

carterkozak · 2023-02-14T16:58:09Z

conjure-java-core/src/test/resources/external-long-test-service.yml

+          externalLong:
+            type: DangerousLong


Let's add a few more args:

declared-safe external imported long

external imported long without any safety information declared

Alias of each of the existing examples

carterkozak · 2023-02-14T17:02:04Z

conjure-java-core/src/test/java/com/palantir/conjure/java/types/ExternalImportSafetyTests.java

+    @Test
+    public void testAliasAnnotations() {
+        assertThat(SafeExternalLongAlias.class).hasAnnotation(Safe.class);
+
+        assertMethodHasAnnotation(SafeExternalLongAlias.class, "toString", Safe.class);
+        assertMethodHasAnnotation(SafeExternalLongAlias.class, "get", Safe.class);
+        assertMethodParamHasAnnotation(SafeExternalLongAlias.class, "valueOf", "value", Safe.class);
+        assertMethodParamHasAnnotation(SafeExternalLongAlias.class, "of", "value", Safe.class);
+    }


I could go either way on reflection-based testing. We have tests which generate integrationInput, so future changes will have clear diffs which can be reviewed more clearly than reading reflection code, but that does rely on the reviewer to understand and account for all changes.

Do you think there's any harm in having both? I see your point about the integrationInput being more readable, but I feel like external import safety is kind of brittle, because if someone forgets to use the SafetyUtils wrappers then we lose safety markings. So I thought a test at least provides some in-your-face protection against future breaks

Totally fine to have both :-)

WIP

b829419

lsingh123 marked this pull request as draft February 2, 2023 16:50

Lavanya Singh added 5 commits February 2, 2023 14:30

adding E2E tests

6ba76b7

Style

8e9717c

Merge branch 'develop' into lsingh/external-imports-log-safety

d3a311b

squash additional direct usages of getSafety

168ee87

fix generated test code

9120f56

lsingh123 closed this Feb 2, 2023

lsingh123 reopened this Feb 8, 2023

Lavanya Singh added 2 commits February 8, 2023 13:51

resurrecting this PR

abdd7d4

Merge remote-tracking branch 'upstream/develop' into lsingh/external-…

bc82bd0

…imports-log-safety

lsingh123 changed the title ~~WIP~~ Generator Should Read ExternalImportType Safety Feb 8, 2023

Lavanya Singh added 4 commits February 8, 2023 17:27

add some tests

2e72ef5

now i know how reflection works i guess

ec54b4c

fix broken test

a3356d1

Merge remote-tracking branch 'upstream/develop' into lsingh/external-…

ba32071

…imports-log-safety

lsingh123 marked this pull request as ready for review February 9, 2023 19:15

lsingh123 commented Feb 9, 2023

View reviewed changes

conjure-java-core/src/integrationInput/java/com/palantir/product/SafeExternalLongExample.java Show resolved Hide resolved

lsingh123 commented Feb 9, 2023

View reviewed changes

conjure-java-core/src/main/java/com/palantir/conjure/java/util/SafetyUtils.java Show resolved Hide resolved

refactoring test

e29e2d8

lsingh123 force-pushed the lsingh/external-imports-log-safety branch from 2c16819 to e29e2d8 Compare February 9, 2023 20:33

Lavanya Singh added 2 commits February 10, 2023 14:46

playing around with wrappers

f05558e

poet :(

ba1f619

lsingh123 commented Feb 10, 2023

View reviewed changes

collections and optionals work now

3c5b062

lsingh123 force-pushed the lsingh/external-imports-log-safety branch from be32776 to 3c5b062 Compare February 10, 2023 21:45

add tests for safetyutils

2b44bfe

lsingh123 commented Feb 13, 2023

View reviewed changes

lsingh123 added no changelog and removed no changelog labels Feb 13, 2023

add changelog

da8b782

lsingh123 force-pushed the lsingh/external-imports-log-safety branch from 92673fe to da8b782 Compare February 13, 2023 19:12

cleaning safety utils

ad0b5ac

carterkozak reviewed Feb 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generator Should Read ExternalImportType Safety #1944

Generator Should Read ExternalImportType Safety #1944

lsingh123 commented Feb 1, 2023 •

edited

Loading

lsingh123 commented Feb 2, 2023

lsingh123 Feb 9, 2023

lsingh123 Feb 10, 2023

carterkozak Feb 10, 2023

lsingh123 Feb 10, 2023

carterkozak Feb 10, 2023

lsingh123 Feb 10, 2023 •

edited

Loading

lsingh123 Feb 10, 2023 •

edited

Loading

lsingh123 Feb 10, 2023 •

edited

Loading

lsingh123 Feb 13, 2023

changelog-app bot commented Feb 13, 2023 •

edited by lsingh123

Loading

carterkozak Feb 14, 2023

carterkozak Feb 14, 2023

lsingh123 Feb 14, 2023

carterkozak Feb 14, 2023

carterkozak Feb 14, 2023

carterkozak Feb 14, 2023

lsingh123 Feb 14, 2023

carterkozak Feb 14, 2023

Generator Should Read ExternalImportType Safety #1944

Are you sure you want to change the base?

Generator Should Read ExternalImportType Safety #1944

Conversation

lsingh123 commented Feb 1, 2023 • edited Loading

lsingh123 commented Feb 2, 2023

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

lsingh123 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

lsingh123 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

lsingh123 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changelog-app bot commented Feb 13, 2023 • edited by lsingh123 Loading

Generate changelog in changelog/@unreleased

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

lsingh123 commented Feb 1, 2023 •

edited

Loading

lsingh123 Feb 10, 2023 •

edited

Loading

lsingh123 Feb 10, 2023 •

edited

Loading

lsingh123 Feb 10, 2023 •

edited

Loading

changelog-app bot commented Feb 13, 2023 •

edited by lsingh123

Loading

Generate changelog in `changelog/@unreleased`