Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion docs/source-2.0/aws/aws-iam.rst
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,11 @@ Each condition key structure supports the following members:
- ``string``
- A relative URL path that defines more information about the condition key
within a set of IAM-related documentation.
* - required
- ``boolean``
- Defines whether a service resolved condition key is required. Not
applicable to request resolved condition keys as the native
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "native @required" should be replaced with a :ref: to the required trait.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
applicable to request resolved condition keys as the native
applicable to request resolved condition keys, as the native

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some sort of validation for if this property is set and the named key is request-resolved. I think that case should fail and direct users to applying the @required trait instead.

Copy link
Contributor

@kstich kstich Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing the :ref: for the required trait and the validation wasn't added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing the :ref: for the required trait and the validation wasn't added.

Added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some sort of validation for if this property is set and the named key is request-resolved. I think that case should fail and direct users to applying the @required trait instead.

Added the validator

@required trait must be used on the field directly.

.. code-block:: smithy

Expand All @@ -448,7 +453,14 @@ Each condition key structure supports the following members:
type: "String"
documentation: "The Bar string"
externalDocumentation: "http://example.com"
})
},
"myservice:Baz": {
type: "String"
documentation: "The Baz string"
externalDocumentation: "http://baz.com"
required: true
}
)
service MyService {
version: "2017-02-11"
resources: [MyResource]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import java.util.Objects;
import java.util.Optional;

import software.amazon.smithy.model.node.BooleanNode;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.node.ObjectNode;
import software.amazon.smithy.model.node.StringNode;
Expand All @@ -29,17 +31,25 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
private static final String DOCUMENTATION = "documentation";
private static final String EXTERNAL_DOCUMENTATION = "externalDocumentation";
private static final String RELATIVE_DOCUMENTATION = "relativeDocumentation";
private static final String REQUIRED = "required";

private final String type;
private final String documentation;
private final String externalDocumentation;
private final String relativeDocumentation;

kstich marked this conversation as resolved.
Show resolved Hide resolved
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be part of the javadoc for the isRequired method and not on the private var.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated javadoc to be on the method

* Whether a service resolved condition key is required
* Not applicable to request resolved condition key as the native @required trait must be used
**/
private final boolean required;

private ConditionKeyDefinition(Builder builder) {
type = SmithyBuilder.requiredState(TYPE, builder.type);
documentation = builder.documentation;
externalDocumentation = builder.externalDocumentation;
relativeDocumentation = builder.relativeDocumentation;
required = builder.required;
}

public static Builder builder() {
Expand All @@ -56,6 +66,8 @@ public static ConditionKeyDefinition fromNode(Node value) {
.ifPresent(builder::externalDocumentation);
objectNode.getStringMember(RELATIVE_DOCUMENTATION).map(StringNode::getValue)
.ifPresent(builder::relativeDocumentation);
objectNode.getBooleanMember(REQUIRED).map(BooleanNode::getValue)
.ifPresent(builder::required);

return builder.build();
}
Expand Down Expand Up @@ -91,13 +103,16 @@ public Optional<String> getRelativeDocumentation() {
return Optional.ofNullable(relativeDocumentation);
}

public Optional<Boolean> getRequired() { return Optional.ofNullable(required); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since required is a primitive boolean, it's not nullable. This should just be a boolean isRequired() method that returns the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be isRequired without nullable


@Override
public SmithyBuilder<ConditionKeyDefinition> toBuilder() {
return builder()
.documentation(documentation)
.externalDocumentation(externalDocumentation)
.relativeDocumentation(relativeDocumentation)
.type(type);
.type(type)
.required(required);
}

@Override
Expand All @@ -107,6 +122,7 @@ public Node toNode() {
.withOptionalMember(DOCUMENTATION, getDocumentation().map(Node::from))
.withOptionalMember(EXTERNAL_DOCUMENTATION, getExternalDocumentation().map(Node::from))
.withOptionalMember(RELATIVE_DOCUMENTATION, getRelativeDocumentation().map(Node::from))
.withOptionalMember(REQUIRED, getRequired().map(Node::from))
.build();
}

Expand All @@ -122,7 +138,8 @@ public boolean equals(Object o) {
return Objects.equals(type, that.type)
&& Objects.equals(documentation, that.documentation)
&& Objects.equals(externalDocumentation, that.externalDocumentation)
&& Objects.equals(relativeDocumentation, that.relativeDocumentation);
&& Objects.equals(relativeDocumentation, that.relativeDocumentation)
&& Objects.equals(required, that.required);
}

@Override
Expand All @@ -135,6 +152,7 @@ public static final class Builder implements SmithyBuilder<ConditionKeyDefinitio
private String documentation;
private String externalDocumentation;
private String relativeDocumentation;
private boolean required;

@Override
public ConditionKeyDefinition build() {
Expand All @@ -160,5 +178,10 @@ public Builder relativeDocumentation(String relativeDocumentation) {
this.relativeDocumentation = relativeDocumentation;
return this;
}

public Builder required(boolean required) {
this.required = required;
return this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ structure ConditionKeyDefinition {
/// A relative URL path that defines more information about the condition key
/// within a set of IAM-related documentation.
relativeDocumentation: String

// Whether a service resolved condition key is required
0xjjoyy marked this conversation as resolved.
Show resolved Hide resolved
// Not applicable to request resolved condition key as the native @required trait must be used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Not applicable to request resolved condition key as the native @required trait must be used
// Not applicable to request resolved condition keys, as the native @required trait must be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

required: Boolean
}

/// Contains information about a resource an IAM action can be authorized against.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package software.amazon.smithy.aws.iam.traits;

import org.junit.jupiter.api.Test;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;

import java.util.Collections;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertFalse;


import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;


public class DefineConditionKeysTraitTest {
@Test
public void loadsFromModel() {
Model result = Model.assembler()
.discoverModels(getClass().getClassLoader())
.addImport(getClass().getResource("define-condition-keys.smithy"))
.assemble()
.unwrap();

Shape shape = result.expectShape(ShapeId.from("smithy.example#MyService"));
DefineConditionKeysTrait trait = shape.expectTrait(DefineConditionKeysTrait.class);
assertEquals(3,trait.getConditionKeys().size());
assertFalse(trait.getConditionKey("myservice:Bar").get().getRequired().get());
assertFalse(trait.getConditionKey("myservice:Foo").get().getRequired().get());
assertTrue(trait.getConditionKey("myservice:Baz").get().getRequired().get());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
$version: "2"

namespace smithy.example

use aws.api#service
use aws.iam#defineConditionKeys

@service(sdkId: "My Value", arnNamespace: "myservice")
@defineConditionKeys(
"myservice:Bar": {
type: "String"
documentation: "The Bar string"
externalDocumentation: "http://example.com"
},
"myservice:Baz": {
type: "String"
documentation: "The Baz string"
externalDocumentation: "http://baz.com"
required: true
}
"myservice:Foo": {
type: "String"
documentation: "The Foo string"
externalDocumentation: "http://foo.com"
required: false
}
)
service MyService {
version: "2017-02-11"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have empty line at EOF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added EOF

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
$version: "2"

namespace smithy.example

use aws.api#service
use aws.iam#defineConditionKeys

@service(sdkId: "My Value", arnNamespace: "myservice")
@defineConditionKeys(
"myservice:Bar": {
type: "String"
documentation: "The Bar string"
externalDocumentation: "http://example.com"
})
service MyService {
version: "2017-02-11"
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[DANGER] -: Syntactic shape ID `trues` does not resolve to a valid shape ID: `smithy.example#trues`. Did you mean to quote this string? Are you missing a model file? | SyntacticShapeIdTarget
[ERROR] smithy.example#MyService: Error creating trait `aws.iam#defineConditionKeys`: Expected `required` to be a boolean; found string | Model
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
$version: "2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test, as it's not validating behavior specific to the trait - it's validation for model loading and parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed


namespace smithy.example

use aws.api#service
use aws.iam#defineConditionKeys

@service(sdkId: "My Value", arnNamespace: "myservice")
@defineConditionKeys(
"myservice:Bar": {
type: "String"
documentation: "The Bar string"
externalDocumentation: "http://example.com"
required: trues
})
service MyService {
version: "2017-02-11"
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
$version: "2"

namespace smithy.example

use aws.api#service
use aws.iam#defineConditionKeys

@service(sdkId: "My Value", arnNamespace: "myservice")
@defineConditionKeys(
"myservice:Bar": {
type: "String"
documentation: "The Bar string"
externalDocumentation: "http://example.com"
required: true
})
service MyService {
version: "2017-02-11"
}

Loading