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

Make sets ordered #875

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions docs/source/1.0/spec/core/constraint-traits.rst
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,11 @@ in a response.
``uniqueItems`` trait
---------------------

.. warning:

This trait has been deprecated. It may be removed in future versions. Set
shapes should be used instead.

Summary
Indicates that the items in a :ref:`list` MUST be unique.
Trait selector
Expand Down
11 changes: 9 additions & 2 deletions docs/source/1.0/spec/core/model.rst
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ reference other shapes using :ref:`members <member>`.
* - :ref:`list`
- Ordered collection of homogeneous values
* - :ref:`set`
- Unordered collection of unique homogeneous values
- Collection of unique homogeneous values
* - :ref:`map`
- Map data structure that maps string keys to homogeneous values
* - :ref:`structure`
Expand Down Expand Up @@ -565,7 +565,7 @@ example is ``smithy.example#MyList$member``.
Set
===

The :dfn:`set` type represents an unordered collection of unique homogeneous
The :dfn:`set` type represents a collection of unique homogeneous
values. A set shape requires a single member named ``member``.
Sets are defined in the IDL using a :ref:`set_statement <idl-set>`.
The following example defines a set of strings:
Expand Down Expand Up @@ -618,6 +618,13 @@ SHOULD represent sets as a custom set data structure that can interpret value
hash codes and equality, or alternatively, store the values of a set data
structure in a list and rely on validation to ensure uniqueness.

.. rubric:: Set member ordering

Sets MUST be insertion ordered. Not all programming languages that support
sets support ordered sets, requiring them may be overly burdensome for users,
or conflict with language idioms. Such languages SHOULD store the values
of sets in a list and rely on validation to ensure uniqueness.


.. _map:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ private Map<ShapeId, Trait> getAuthTraitValues(Shape service, Shape subject) {

AuthTrait authTrait = subject.expectTrait(AuthTrait.class);
Map<ShapeId, Trait> result = new LinkedHashMap<>();
for (ShapeId value : authTrait.getValues()) {
for (ShapeId value : authTrait.getValueSet()) {
service.findTrait(value).ifPresent(trait -> result.put(value, trait));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -174,9 +175,11 @@ private ReflectiveSupplier<Collection<Object>> createSupplier(Class<?> targetTyp
|| targetType.equals(ArrayList.class)
|| targetType.equals(Iterable.class)) {
return ArrayList::new;
} else if (targetType.equals(Set.class) || targetType.equals(HashSet.class)) {
} else if (targetType.equals(Set.class)
|| targetType.equals(HashSet.class)
|| targetType.equals(LinkedHashSet.class)) {
// Special casing for Set or HashSet.
return HashSet::new;
return LinkedHashSet::new;
} else if (Collection.class.isAssignableFrom(targetType)) {
return createSupplierFromReflection(targetType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@

package software.amazon.smithy.model.traits;

import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import software.amazon.smithy.model.FromSourceLocation;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.node.ArrayNode;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.node.StringNode;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.SetUtils;

/**
* Specifies the auth schemes supported by default for operations
Expand All @@ -33,34 +35,50 @@ public final class AuthTrait extends AbstractTrait {

public static final ShapeId ID = ShapeId.from("smithy.api#auth");

private final List<ShapeId> values;
private final Set<ShapeId> values;

@Deprecated
public AuthTrait(List<ShapeId> values, FromSourceLocation sourceLocation) {
super(ID, sourceLocation);
this.values = ListUtils.copyOf(values);
this.values = SetUtils.orderedCopyOf(values);
}

@Deprecated
public AuthTrait(List<ShapeId> values) {
this(values, SourceLocation.NONE);
}

public AuthTrait(Set<ShapeId> values, FromSourceLocation sourceLocation) {
super(ID, sourceLocation);
this.values = SetUtils.orderedCopyOf(values);
}

public AuthTrait(Set<ShapeId> values) {
this(values, SourceLocation.NONE);
}

/**
* Gets the auth scheme trait values.
*
* @return Returns the supported auth schemes.
*/
public List<ShapeId> getValues() {
public Set<ShapeId> getValueSet() {
return values;
}

@Deprecated
public List<ShapeId> getValues() {
return ListUtils.copyOf(values);
}

public static final class Provider extends AbstractTrait.Provider {
public Provider() {
super(ID);
}

@Override
public Trait createTrait(ShapeId target, Node value) {
List<ShapeId> values = new ArrayList<>();
Set<ShapeId> values = new LinkedHashSet<>();
for (StringNode node : value.expectArrayNode().getElementsAs(StringNode.class)) {
values.add(node.expectShapeId());
}
Expand All @@ -70,7 +88,7 @@ public Trait createTrait(ShapeId target, Node value) {

@Override
protected Node createNode() {
return getValues().stream().map(ShapeId::toString).map(Node::from)
return getValueSet().stream().map(ShapeId::toString).map(Node::from)
.collect(ArrayNode.collect(getSourceLocation()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

package software.amazon.smithy.model.traits;

import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -121,12 +121,12 @@ public Builder maxAge(int maxAge) {
}

public Builder additionalAllowedHeaders(Set<String> additionalAllowedHeaders) {
this.additionalAllowedHeaders = new HashSet<>(Objects.requireNonNull(additionalAllowedHeaders));
this.additionalAllowedHeaders = new LinkedHashSet<>(Objects.requireNonNull(additionalAllowedHeaders));
return this;
}

public Builder additionalExposedHeaders(Set<String> additionalExposedHeaders) {
this.additionalExposedHeaders = new HashSet<>(Objects.requireNonNull(additionalExposedHeaders));
this.additionalExposedHeaders = new LinkedHashSet<>(Objects.requireNonNull(additionalExposedHeaders));
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
/**
* Indicates that the members of a list must be unique.
*/
@Deprecated
public final class UniqueItemsTrait extends AnnotationTrait {
public static final ShapeId ID = ShapeId.from("smithy.api#uniqueItems");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ private void validateShape(
) {
if (shape.getTrait(AuthTrait.class).isPresent()) {
AuthTrait authTrait = shape.getTrait(AuthTrait.class).get();
Set<ShapeId> appliedAuthTraitValue = new TreeSet<>(authTrait.getValues());
Set<ShapeId> appliedAuthTraitValue = new TreeSet<>(authTrait.getValueSet());
appliedAuthTraitValue.removeAll(serviceAuth);

if (!appliedAuthTraitValue.isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ map externalDocumentation {

/// Defines the list of authentication schemes supported by a service or operation.
@trait(selector: ":is(service, operation)")
@uniqueItems
list auth {
set auth {
member: AuthTraitReference
}

Expand Down Expand Up @@ -492,6 +491,7 @@ structure sparse {}

/// Indicates that the items in a list MUST be unique.
@trait(selector: "list")
@deprecated(message: "The uniqueItems trait has been deprecated in favor of using sets.", since: "2.0")
JordonPhillips marked this conversation as resolved.
Show resolved Hide resolved
structure uniqueItems {}

/// Indicates that the shape is unstable and could change in the future.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ public void deserializesSets() {
NodeMapper mapper = new NodeMapper();

Set<String> result = mapper.deserializeCollection(value, Set.class, String.class);
assertThat(result, instanceOf(HashSet.class));
assertThat(result, instanceOf(LinkedHashSet.class));
assertThat(result, contains("a", "b"));
}

Expand All @@ -794,7 +794,7 @@ public void deserializesHashSets() {
NodeMapper mapper = new NodeMapper();

Set<String> result = mapper.deserializeCollection(value, HashSet.class, String.class);
assertThat(result, instanceOf(HashSet.class));
assertThat(result, instanceOf(LinkedHashSet.class));
assertThat(result, contains("a", "b"));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ private <T extends Trait> void addOperationSecurity(
// If the operation explicitly removes authentication, ensure that "security" is set to an empty
// list as opposed to simply being unset as unset will result in the operation inheriting global
// configuration.
if (shape.getTrait(AuthTrait.class).map(trait -> trait.getValues().isEmpty()).orElse(false)) {
if (shape.getTrait(AuthTrait.class).map(trait -> trait.getValueSet().isEmpty()).orElse(false)) {
builder.security(Collections.emptyList());
return;
}
Expand Down