Skip to content

Stop gating identification on IA.properties identificationClassN - #1778

Merged
JasonWildMe merged 1 commit into
mainfrom
fix/remove-identificationclass-gate
Oct 1, 2026
Merged

JasonWildMe merged 1 commit into
mainfrom
fix/remove-identificationclass-gate

Conversation

@JasonWildMe

Copy link
Copy Markdown
Collaborator

Problem

IBEISIA.validForIdentification(ann, context) rejected any non-trivial annotation whose iaClass was missing from the legacy IA.properties identificationClassN allowlist. Species and class configuration moved to IA.json long ago, so the list goes stale without anyone noticing. When it's stale, annotations drop out of identification with only a NOTE: log line to show for it.

Of the 280 local and origin branches surveyed, only two install branches define the key:

  • amphibian-reptile sets identificationClass0..2 = fire_sal, salanader_fire, salanader_fire_adult, a 2021 fire-salamander template. The install now configures about 35 taxa in IA.json (frog, toad, lizard, caiman, …). HotSpotter silently skips every one of them that isn't a salamander.
  • iot sets identificationClass0 = and identificationClass1 =. Properties.getProperty returns "" for a blank value, so the list becomes ["", ""]. That list is non-empty, so every annotation with a real class fails the gate.

main and every other install don't define the key. On those the gate already lets everything through, and this PR changes nothing for them.

Change

  • Remove the class-allowlist gate from validForIdentification. The null-annotation and bbox checks stay, and the AnnotationLite cache behavior is unchanged. Both overloads keep their signatures, so no caller changes.
  • Keep validIAClassForIdentification (both overloads) and getAllIdentificationClasses with their behavior unchanged, marked @Deprecated, for any external or custom-JSP callers.
  • Correct the AcmIdBot comment that described the old check.
  • New IBEISIAValidForIdentificationTest:
    • Covers a stale list, a blank-value list, a null annotation, and an annotation with no bbox.
    • The stale and blank cases fail on the old code and pass after the change.
    • No database needed; the static mock of IA.getProperty is scoped with try-with-resources.

No replacement eligibility gate is added. The existing path-specific IA.json checks remain, for example isValidIAClass before setting matchAgainst in convertAnnotation and manualAnnotation.jsp.

Behavior change on installs that define the list

On an install whose mounted IA.properties defines identificationClassN, annotations of classes not in that list now pass validForIdentification. The affected paths are:

  • WBIA identify (sendIdentify and beginIdentifyAnnotations, the HotSpotter path)
  • intake in processCallback
  • matchAgainst assignment in convertAnnotation and manualAnnotation.jsp, still gated by IA.json
  • AcmIdBot registration
  • __sendAnnotations
  • GetCurrentIAInfo?onlyIdentifiable=true

The v2 ml-service / MiewID path and React manual annotation never used this gate and are unaffected.

Deploy notes

  • Before deploying, run grep identificationClass on each install's mounted WEB-INF/classes/bundles/IA.properties. The branch copies were surveyed; the mounted files may differ. Any install that used the list to deliberately exclude a class should move that exclusion into IA.json first.
  • Restart Tomcat after deploying. AnnotationLite caches per-annotation validForIdentification=false results in memory, and the restart clears the stale ones.
  • The leftover identificationClassN lines in IA.properties are now ignored and can be removed whenever it's convenient.

Testing

  • mvn test: 1067 tests, 0 failures, 0 errors, 7 skipped.
  • IBEISIAValidForIdentificationTest fails on the old code (2 assertion failures) and passes after the change (4/4).
  • AcmIdBot*Test: 32/32 pass.

🤖 Generated with Claude Code
Reviewed with Codex (GPT-6 Astra): 2 plan-review rounds and 1 code-review round, converged with no Critical or Major findings.

IBEISIA.validForIdentification(ann, context) rejected any non-trivial
annotation whose iaClass was not in the legacy IA.properties
identificationClassN allowlist. Identification config lives in IA.json,
so a stale list (amphibian-reptile: a fire-salamander template) or blank
entries (iot: "identificationClass0 =", read as "") silently dropped
annotations from WBIA/HotSpotter identify, matchAgainst assignment in
convertAnnotation/manualAnnotation.jsp, and AcmIdBot registration.

Remove the gate; validForIdentification keeps its null and bbox checks.
The allowlist helpers are kept unchanged and marked @deprecated for
external callers. No replacement eligibility gate is introduced;
existing path-specific IA.json checks remain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex (GPT-6 Astra) <noreply@openai.com>
@JasonWildMe JasonWildMe added this to the 11.0 milestone Sep 25, 2026
@JasonWildMe

Copy link
Copy Markdown
Collaborator Author

Review record

Codex round 1 (plan): not converged. Findings:

  • Removing the gate lets more annotations through on WBIA paths, and IA.json doesn't fully replace it:
    • isValidIAClass only checks that a class exists in IA.json, not that it has an _id_conf.
    • AcmIdBot registration and sendIdentify have no IA.json check at all.
  • Deleting the three public helpers could break custom JSPs mounted on installs.
  • The test fixture needed an explicit FeatureType, a MediaAsset attachment, and non-trivial dimensions, or the test would pass without exercising the gate.

Response: A survey of all 280 branches found only two installs that define the key. Neither uses it as a deliberate exclusion:

  • amphibian-reptile has a stale salamander template.
  • iot has blank values, which reject everything.

I kept the helpers @Deprecated with their behavior unchanged, adopted the fixture fixes, and listed every path that now lets more through in the PR body.

Codex round 2 (plan): converged. One Minor: the Javadoc and PR wording implied IA.json becomes a universal gate. I reworded it.

Codex round 3 (code): converged, no findings.

Caught by the test run, not by review: validForIdentification stringifies features, and MediaAsset.toString() throws a NullPointerException without an AssetStore. The fixture now uses a mocked store. That's why the first RED run failed with errors rather than assertion failures.

This record isn't a claim that the change doesn't need human review.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.84%. Comparing base (dcf6f46) to head (f0c7f47).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1778   +/-   ##
=======================================
  Coverage   54.84%   54.84%           
=======================================
  Files         314      314           
  Lines       12704    12704           
  Branches     4006     4095   +89     
=======================================
  Hits         6968     6968           
+ Misses       5441     5435    -6     
- Partials      295      301    +6     
Flag Coverage Δ
backend 54.84% <ø> (ø)
frontend 54.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JasonWildMe JasonWildMe self-assigned this Sep 26, 2026

@naknomum naknomum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@JasonWildMe
JasonWildMe merged commit 7da561c into main Oct 1, 2026
2 checks passed
@JasonWildMe
JasonWildMe deleted the fix/remove-identificationclass-gate branch October 1, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants