Implement endpoint argument safety declarations (#1153)

Implement consistent endpoint argument safety declarations matching field, alias, and union safety

changelog/@unreleased/pr-1153.v2.yml

@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: Implement consistent endpoint argument safety declarations matching
+    field, alias, and union safety
+  links:
+  - https://github.com/palantir/conjure/pull/1153

conjure-api/src/main/conjure/conjure-api.yml

@@ -200,6 +200,7 @@ types:
           argName: ArgumentName
           type: Type
           paramType: ParameterType
+          safety: optional<LogSafety>
           docs: optional<Documentation>
           markers: list<Type>
           tags: set<string>

conjure-core/src/main/java/com/palantir/conjure/defs/ConjureParserUtils.java

@@ -470,6 +470,7 @@ private static List<ArgumentDefinition> parseArgs(
                     .type(original.type().visit(new ConjureTypeParserVisitor(typeResolver)))
                     .paramType(paramType)
                     .docs(original.docs().map(Documentation::of))
+                    .safety(original.safety().map(ConjureParserUtils::parseLogSafety))
                     .markers(parseMarkers(original.markers(), typeResolver))
                     .tags(original.tags().stream()
                             .peek(tag -> Preconditions.checkArgument(!tag.isEmpty(), "tag must not be empty"))

conjure-core/src/main/java/com/palantir/conjure/defs/validator/ConjureDefinitionValidator.java

@@ -436,6 +436,23 @@ private static final class LogSafetyConjureDefinitionValidator implements Conjur
         @Override
         public void validate(ConjureDefinition definition) {
             definition.getTypes().forEach(SafetyValidator::validate);
+            definition.getServices().forEach(serviceDefinition -> serviceDefinition
+                    .getEndpoints()
+                    .forEach(endpointDefinition -> endpointDefinition.getArgs().forEach(argumentDefinition -> {
+                        try {
+                            SafetyValidator.validateDefinition(
+                                    argumentDefinition.getSafety(), argumentDefinition.getType());
+                        } catch (RuntimeException e) {
+                            throw new ConjureIllegalStateException(
+                                    String.format(
+                                            "%s.%s(%s): %s",
+                                            serviceDefinition.getServiceName().getName(),
+                                            endpointDefinition.getEndpointName(),
+                                            argumentDefinition.getArgName(),
+                                            e.getMessage()),
+                                    e);
+                        }
+                    })));
         }
     }
 }

conjure-core/src/main/java/com/palantir/conjure/defs/validator/SafetyValidator.java

@@ -41,11 +41,17 @@ public static void validate(TypeDefinition type) {
         type.accept(TypeDefinitionSafetyVisitor.INSTANCE);
     }
 
-    private static ConjureIllegalStateException fail(Object typeDescription) {
+    public static void validateDefinition(Optional<LogSafety> declaredSafety, Type type) {
+        if (declaredSafety.isPresent()) {
+            type.accept(SafetyTypeVisitor.INSTANCE);
+        }
+    }
+
+    private static ConjureIllegalStateException fail(TypeName typeName) {
         return new ConjureIllegalStateException(String.format(
-                "%s cannot declare log safety. Only conjure primitives and "
+                "%s.%s cannot declare log safety. Only conjure primitives and "
                         + "wrappers around conjure primitives may declare safety.",
-                typeDescription));
+                typeName.getPackage(), typeName.getName()));
     }
 
     private enum TypeDefinitionSafetyVisitor implements TypeDefinition.Visitor<Void> {

conjure-core/src/main/java/com/palantir/conjure/exceptions/ConjureIllegalStateException.java

@@ -20,4 +20,8 @@ public class ConjureIllegalStateException extends IllegalStateException implemen
     public ConjureIllegalStateException(String reason) {
         super(reason);
     }
+
+    public ConjureIllegalStateException(String reason, Throwable cause) {
+        super(reason, cause);
+    }
 }

conjure-core/src/main/java/com/palantir/conjure/parser/services/ArgumentDefinition.java

@@ -24,6 +24,7 @@
 import com.fasterxml.jackson.databind.JsonDeserializer;
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.palantir.conjure.defs.ConjureImmutablesStyle;
+import com.palantir.conjure.parser.LogSafetyDefinition;
 import com.palantir.conjure.parser.services.ArgumentDefinition.ArgumentDefinitionDeserializer;
 import com.palantir.conjure.parser.types.ConjureType;
 import com.palantir.parsec.ParseException;
@@ -51,6 +52,8 @@ default ParamType paramType() {
         return ParamType.AUTO;
     }
 
+    Optional<LogSafetyDefinition> safety();
+
     Set<ConjureType> markers();
 
     Set<String> tags();

conjure-core/src/test/java/com/palantir/conjure/defs/validator/ConjureSourceFileValidatorTest.java

@@ -21,6 +21,9 @@
 
 import com.google.common.collect.ImmutableList;
 import com.palantir.conjure.spec.AliasDefinition;
+import com.palantir.conjure.spec.ArgumentDefinition;
+import com.palantir.conjure.spec.ArgumentName;
+import com.palantir.conjure.spec.BodyParameterType;
 import com.palantir.conjure.spec.ConjureDefinition;
 import com.palantir.conjure.spec.Documentation;
 import com.palantir.conjure.spec.EndpointDefinition;
@@ -35,6 +38,7 @@
 import com.palantir.conjure.spec.MapType;
 import com.palantir.conjure.spec.ObjectDefinition;
 import com.palantir.conjure.spec.OptionalType;
+import com.palantir.conjure.spec.ParameterType;
 import com.palantir.conjure.spec.PrimitiveType;
 import com.palantir.conjure.spec.ServiceDefinition;
 import com.palantir.conjure.spec.SetType;
@@ -298,6 +302,81 @@ public void testSafetyOnMap() {
                 .hasMessageContainingAll("Maps cannot declare log safety", "Consider using alias types");
     }
 
+    @Test
+    public void testInvalidSafetyArgument_bearertoken() {
+        ConjureDefinition conjureDef = ConjureDefinition.builder()
+                .version(1)
+                .services(ServiceDefinition.builder()
+                        .serviceName(TypeName.of("Service", "com.palantir.product"))
+                        .endpoints(EndpointDefinition.builder()
+                                .endpointName(EndpointName.of("end"))
+                                .httpMethod(HttpMethod.PUT)
+                                .httpPath(HttpPath.of("/path"))
+                                .args(ArgumentDefinition.builder()
+                                        .argName(ArgumentName.of("arg"))
+                                        .type(Type.primitive(PrimitiveType.BEARERTOKEN))
+                                        .paramType(ParameterType.body(BodyParameterType.of()))
+                                        .safety(LogSafety.UNSAFE)
+                                        .build())
+                                .build())
+                        .build())
+                .build();
+        assertThatThrownBy(() -> ConjureDefinitionValidator.validateAll(conjureDef))
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessageContaining(
+                        "Service.end(arg): bearertoken values are do-not-log by default and cannot be configured");
+    }
+
+    @Test
+    public void testInvalidSafetyArgument_reference() {
+        ConjureDefinition conjureDef = ConjureDefinition.builder()
+                .version(1)
+                .types(TypeDefinition.object(
+                        ObjectDefinition.builder().typeName(FOO).build()))
+                .services(ServiceDefinition.builder()
+                        .serviceName(TypeName.of("Service", "com.palantir.product"))
+                        .endpoints(EndpointDefinition.builder()
+                                .endpointName(EndpointName.of("end"))
+                                .httpMethod(HttpMethod.PUT)
+                                .httpPath(HttpPath.of("/path"))
+                                .args(ArgumentDefinition.builder()
+                                        .argName(ArgumentName.of("arg"))
+                                        .type(Type.reference(FOO))
+                                        .paramType(ParameterType.body(BodyParameterType.of()))
+                                        .safety(LogSafety.UNSAFE)
+                                        .build())
+                                .build())
+                        .build())
+                .build();
+        assertThatThrownBy(() -> ConjureDefinitionValidator.validateAll(conjureDef))
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessageContaining(
+                        "Service.end(arg): package.Foo cannot declare log safety. Only conjure primitives and wrappers"
+                                + " around conjure primitives may declare safety.");
+    }
+
+    @Test
+    public void testValidArgumentSafety() {
+        ConjureDefinition conjureDef = ConjureDefinition.builder()
+                .version(1)
+                .services(ServiceDefinition.builder()
+                        .serviceName(TypeName.of("Service", "com.palantir.product"))
+                        .endpoints(EndpointDefinition.builder()
+                                .endpointName(EndpointName.of("end"))
+                                .httpMethod(HttpMethod.PUT)
+                                .httpPath(HttpPath.of("/path"))
+                                .args(ArgumentDefinition.builder()
+                                        .argName(ArgumentName.of("arg"))
+                                        .type(Type.primitive(PrimitiveType.STRING))
+                                        .paramType(ParameterType.body(BodyParameterType.of()))
+                                        .safety(LogSafety.UNSAFE)
+                                        .build())
+                                .build())
+                        .build())
+                .build();
+        ConjureDefinitionValidator.validateAll(conjureDef);
+    }
+
     @Test
     public void testNoSafetyOnPrimitive() {
         ConjureDefinition conjureDef = ConjureDefinition.builder()

docs/spec/conjure_definitions.md

@@ -371,14 +371,13 @@ An object representing an argument to an endpoint.
 
 Field | Type | Description
 ---|:---:|---
-type | [ConjureType][] | **REQUIRED**. The type of the value of the argument. The type name MUST exist within the Conjure definition. If this ArgumentDefinition has a param-type of `body` then there are no restrictions on the type. 
-If the param-type is `path` then the de-aliased type MUST be an enum or a primitive (except `binary`, and `bearertoken`).
-If the param-type is `query` then the de-aliased type MUST be an enum or a primitive (except `binary` and `bearertoken`), or a container (list, set, optional) of one of these.
-If the param-type is `header` then the de-aliased type MUST be an enum or a primitive (except binary), or an optional of one of these.
-markers | List[`string`] | List of types that serve as additional metadata for the argument. If the value of the field is a `string` it MUST be a type name that exists within the Conjure definition. Prefer to use tags instead of markers.
-tags | Set[`string`] | Set of tags that serves as additional metadata for the argument.
+type | [ConjureType][] | **REQUIRED**. The type of the value of the argument. The type name MUST exist within the Conjure definition. If this ArgumentDefinition has a param-type of `body` then there are no restrictions on the type. If the param-type is `path` then the de-aliased type MUST be an enum or a primitive (except `binary`, and `bearertoken`). If the param-type is `query` then the de-aliased type MUST be an enum or a primitive (except `binary` and `bearertoken`), or a container (list, set, optional) of one of these. If the param-type is `header` then the de-aliased type MUST be an enum or a primitive (except binary), or an optional of one of these.
 param‑id | `string` | An identifier to use as a parameter value. If the param type is `header` or `query`, this field may be populated to define the identifier that is used over the wire. If this field is undefined for the `header` or `query` param types, the argument name is used as the wire identifier. Population of this field is invalid if the param type is not `header` or `query`.
 param‑type | [ArgumentDefinition.ParamType][] | The type of the endpoint parameter. If omitted the default type is `auto`.
+safety |  [LogSafety][]  | The safety of the type in regards to logging in accordance with the SLS specification. Allowed values are `safe`, `unsafe`, and `do-not-log`. Only conjure primitives, and wrappers around conjure primitives may declare safety, the safety of complex types is computed based on the type graph.
+docs |  [DocString][]  | Documentation for the argument. [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation.
+tags | Set[`string`] | Set of tags that serves as additional metadata for the argument.
+markers | List[`string`] | **DEPRECATED**. List of types that serve as additional metadata for the argument. If the value of the field is a `string` it MUST be a type name that exists within the Conjure definition. Prefer to use tags instead of markers.
 
 Arguments with parameter type `body` MUST NOT be of type `optional<binary>`, or, intuitively, of a type that reduces to `optional<binary>` via unfolding of alias definitions and nested `optional`.
 
@@ -435,30 +434,47 @@ The exception is that `bearertoken` is always considered `do-not-log` and safety
 
 For example, the following 
 ```yaml
-MySimpleAlias:
-  alias: string
-  safety: safe
-MyWrapperAlias:
-  alias: list<optional<string>>
-  safety: unsafe
-MyObject:
-  fields:
-    wrappedPrimitive:
-      type: set<rid>
-      safety: do-not-log
-    token:
-      # Safety cannot be declared for bearer-tokens, these are always
-      # considered 'do-not-log' because the bearer token is a credential
-      # type.
-      type: bearertoken
-    reference:
-      # Safety cannot be declared here, MySimpleAlias is responsible for
-      # declaring safety.
-      type: MySimpleAlias
-    mapField:
-      # Safety cannot be declared on maps, or wrappers around maps.
-      # Consider using alias types to declare safety in a way that
-      # integrates with the type system
-      # e.g. map<MySimpleAlias,MySimpleAlias>
-      type: map<string,string>
+types:
+  definitions:
+    default-package: com.palantir.product
+    objects:
+      MySimpleAlias:
+        alias: string
+        safety: safe
+      MyWrapperAlias:
+        alias: list<optional<string>>
+        safety: unsafe
+      MyObject:
+        fields:
+          wrappedPrimitive:
+            type: set<rid>
+            safety: do-not-log
+          token:
+            # Safety cannot be declared for bearer-tokens, these are always
+            # considered 'do-not-log' because the bearer token is a credential
+            # type.
+            type: bearertoken
+          reference:
+            # Safety cannot be declared here, MySimpleAlias is responsible for
+            # declaring safety.
+            type: MySimpleAlias
+          mapField:
+            # Safety cannot be declared on maps, or wrappers around maps.
+            # Consider using alias types to declare safety in a way that
+            # integrates with the type system
+            # e.g. map<MySimpleAlias,MySimpleAlias>
+            type: map<string,string>
+services:
+  MyService:
+    package: com.palantir.product
+    endpoints:
+      send:
+        http: POST /send
+        args:
+          arg:
+            type: string
+            param-type: body
+            # Safety may be declared on primitive endpoint arguments as well as
+            # aliases, object fields, and union types.
+            safety: safe
 ```