Feature Request: ArchRule for Comparable HashMap keys

[MahatmaFatalError]

Reference: https://stackoverflow.com/questions/70507647/how-to-assert-hashmap-keys-to-be-comparable-with-archunit/70509141

VU#903934 (Hash table implementations vulnerable to algorithmic complexity attacks) Fix with JEP180 in Java8: if too many hash codes map to the same bucket in the map, the list of entries can be changed into a balanced binary tree, sorted first by hash code and then by each key’s compareTo method, as long as the keys are Comparable.

IMHO it makes sense to provide such a rule in the standard ruleset so that others can benefit from it easily.

	@ArchTag("DoS-Protection")
	@ArchTest
	static final ArchRule fields_of_type_HashMap_should_have_Comparable_key = fields().that()
			.haveRawType(Map.class)
			.should(haveComparableFirstTypeParameter());

	@ArchTag("DoS-Protection")
	@ArchTest
	static final ArchRule code_units_should_have_parameters_of_type_Map_with_Comparable_key = codeUnits()
			.should(new ArchCondition<JavaCodeUnit>("have parameters of type Map with `Comparable` key") {
				@Override
				public void check(final JavaCodeUnit javaCodeUnit, final ConditionEvents events) {
					javaCodeUnit.getParameters().forEach(parameter -> {
						if (parameter.getRawType().isEquivalentTo(Map.class)) {
							haveComparableFirstTypeParameter().check(parameter, events);
						}
					});
				}
			});

	@ArchTag("DoS-Protection")
	@ArchTest
	static final ArchRule methods_with_return_type_Map_should_have_return_types_with_Comparable_key = methods().that()
			.haveRawReturnType(Map.class)
			.should(new ArchCondition<JavaMethod>("have return type with `Comparable` key") {
				@Override
				public void check(final JavaMethod method, final ConditionEvents events) {
					class ReturnType implements HasType, HasDescription {
						@Override
						public JavaType getType() {
							return method.getReturnType();
						}

						@Override
						public JavaClass getRawType() {
							return method.getRawReturnType();
						}

						@Override
						public String getDescription() {
							return "Return type <" + this.getType().getName() + "> of " + method.getDescription();
						}
					}
					haveComparableFirstTypeParameter().check(new ReturnType(), events);
				}
			});

	private static <T extends HasType & HasDescription> ArchCondition<T> haveComparableFirstTypeParameter() {
		return new ArchCondition<>("have `Comparable` first type parameter") {
			@Override
			public void check(final T typed, final ConditionEvents events) {
				final JavaType fieldType = typed.getType();
				if (fieldType instanceof JavaParameterizedType) {
					final JavaType keyType = ((JavaParameterizedType) fieldType).getActualTypeArguments().get(0);

					if (keyType instanceof JavaTypeVariable) {
						final boolean isGenericType = true;
						final String message = String.format(
								"%s has a generic first type parameter %s. Comparable check skipped.",
								typed.getDescription(), keyType.getName());
						events.add(new SimpleConditionEvent(typed, isGenericType, message));
					} else {
						final JavaClass erasedType = keyType.toErasure();

						final boolean isComparable = erasedType.getAllRawInterfaces()
								.stream()
								.anyMatch(rawInterface -> rawInterface.isEquivalentTo(Comparable.class));
						final String message = String.format("%s has a first type parameter %s that %s Comparable",
								typed.getDescription(), keyType.getName(), isComparable ? "is" : "is not");
						events.add(new SimpleConditionEvent(typed, isComparable, message));
					}

				} else {
					events.add(SimpleConditionEvent.violated(typed, typed.getDescription() + " is not parameterized"));
				}
			}
		};
	}

However, for a complete check, this https://github.com/TNG/ArchUnit/issues/768 feature needs to be implemented first.

[codecholeric]

Thanks a lot for sharing this :smiley: Yes, we could definitely add this to the predefined rules, I just wonder if we should already do this without #768? Or do you think it doesn't provide enough value then? I kind of think most such relevant errors would be caught by the rule though, because the only maps left are then in a narrow local scope which probably reduces the risk of a targeted attack a lot, don't you think? If we put it to the library I would make it one rule though and explain the issue behind the rule.

[MahatmaFatalError]

I kind of think most such relevant errors would be caught by the rule though, because the only maps left are then in a narrow local scope which probably reduces the risk of a targeted attack a lot, don't you think?

Yes, agree.

If we put it to the library I would make it one rule though and explain the issue behind the rule.

Yes, makes sense.

Thanks for taking care 👍