apache / apache/texera

Attribute.equals has unreachable null guards, and the second ignores the attribute names

Open
#8,149 1 comment 0 reactions 2 assignees Claimed by @aglinxinyuan View on GitHub
Dominant language
Scala
Stars
314
Forks
187
Avg merge
1d 21h
Merged PRs (30d)
214

Description

### What happened?

`Attribute.equals` has two null guards that are unreachable, and the second one is wrong if it ever *were* reached.

```java
Attribute that = (Attribute) toCompare;

if (this.attributeName == null) {
return that.attributeName == null;
}
if (this.attributeType == null) {
return that.attributeType == null; // ignores the names entirely
}

return this.attributeName.equalsIgnoreCase(that.attributeName)
&& this.attributeType.equals(that.attributeType);
```

**They cannot be reached.** The only constructor `checkNotNull`s both fields:

```java
public Attribute(String attributeName, AttributeType attributeType) {
checkNotNull(attributeName);
checkNotNull(attributeType);
...
}
```

**The second guard is also a latent bug.** If `attributeType` were ever null — say a future constructor, a deserialization path, or reflection — then `equals` returns `that.attributeType == null` **without comparing the names at all**. Two attributes with different names and null types would compare equal, which breaks the `Schema` lookups and `Set` semantics built on this class.

The safe fix is to delete both guards, since the constructor already guarantees non-null. If the guards are meant to survive, the second must still compare names.

Found while assessing test coverage; these lines are among the file's uncovered residue. **Deliberately not pinned by a test** — a test would have to construct an instance the constructor forbids, and pinning the current second guard would cement the name-ignoring comparison.

Note the file is a `.java` file living under `src/main/scala/`, which is easy to miss when searching.

### How to reproduce?

Static:

1. `common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java` — the constructor's two `checkNotNull` calls, then the two null guards in `equals`.
2. Coverage on `main` shows those guard lines and their bodies as never executed.

### Version/Branch

1.3.0-incubating-SNAPSHOT (main)

### Was this issue authored using generative AI tooling?

Generated-by: Claude Code (Opus 5)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.