external imports have safety now #1966
Conversation
Generate changelog in
|
| checkNotBuilt(); | ||
| this.safeExternalLongList.add(safeExternalLongList); | ||
| return this; | ||
| } | ||
|
|
||
| @JsonSetter(value = "safeExternalLongSet", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) | ||
| public Builder safeExternalLongSet(@Nonnull Iterable<? extends Long> safeExternalLongSet) { | ||
| public Builder safeExternalLongSet(@Safe @Nonnull Iterable<? extends Long> safeExternalLongSet) { |
There was a problem hiding this comment.
ideally this would be @Nonnull Iterable<@Safe ? extends Long> but there is a bug in poet that makes that impossible. i think this is a pretty good workaround.
There was a problem hiding this comment.
👍 looks reasonable to me
conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java
Outdated
Show resolved
Hide resolved
conjure-java-core/src/integrationInput/java/com/palantir/product/SafeExternalLongExample.java
Show resolved
Hide resolved
conjure-java-core/src/main/java/com/palantir/conjure/java/types/AliasGenerator.java
Outdated
Show resolved
Hide resolved
conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanGenerator.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static boolean isBoxedPrimitive(TypeName type) { | ||
| return type.withoutAnnotations().isBoxedPrimitive(); |
There was a problem hiding this comment.
poet's isBoxedPrimitive method uses object equality but its isPrimitive method uses reference equality
|
|
||
| // SafetyEvaluator uses its TypeDefinitionMap to resolve the safety of reference types, but these utils | ||
| // only evaluate the safety of external imports and primitives, so it's safe to use a placeholder | ||
| private static final SafetyEvaluator safetyEvaluator = new SafetyEvaluator(ImmutableMap.of()); |
There was a problem hiding this comment.
As al alternative, we could make these methods non-static and add them to the SafetyEvaluator -- that may be more robust (or simply add the static methods to SafetyEvaluator so the safety evaluation pieces live in the same place)
There was a problem hiding this comment.
I was trying to avoid plumbing through a SafetyEvaluator instance everywhere these utils are used, but it's probably a good idea to do so
There was a problem hiding this comment.
I think we already have one plumbed in a bunch of places where it's not actually used, heh. If that turns out to require a great deal of plumbing, we could use static methods on SafetyEvaluator, and defer the plumbing to a follow-up commit. Up to you.
|
|
||
| public final class PrimitiveHelpers { | ||
|
|
||
| private PrimitiveHelpers() {} |
There was a problem hiding this comment.
Pedantic naming nit: Rather than Helper or Util prefixes, we generally use plural of the target, e.g. Primitives.
| @@ -78,6 +79,27 @@ public Optional<LogSafety> evaluate(Type type, Optional<LogSafety> declaredSafet | |||
| return declaredSafety.or(() -> evaluate(type)); | |||
| } | |||
|
|
|||
| public Optional<LogSafety> getUsageTimeSafety(ArgumentDefinition argument) { | |||
There was a problem hiding this comment.
We should add some javadoc to these methods since it's not entirely obvious when they should be used without the full context of this change
|
Aside from the javadoc request, this looks good to me -- great work! |
|
Released 6.73.0 |
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.
Tested locally against two large internal repos with complex conjure definitions.