OHDSI / OHDSI/WebAPI

GET /conceptset/{id}/expression returns items in nondeterministic order (query has no ORDER BY)

Open
#2,543 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
151
Forks
183
Avg merge
14m
Merged PRs (30d)
2

Description

Summary

GET /conceptset/{id}/expression can return the items of an unmodified concept set in a different order across identical requests. The response content is otherwise identical — same concepts, same flags — only the order of expression.items differs.

This has been confirmed first-hand against a live ATLAS instance.

Environment

  • ATLAS 2.15.0, WebAPI 2.15.1
  • Also present on 2.14, so this is not a regression introduced in 2.15 — it looks like long-standing behavior, which is consistent with the code history below (the relevant query and schema have been unchanged since the original migrations).

Observed: repeated GET /conceptset/{id}/expression for the same concept set, with no edit to that concept set in between, returned items in different orders.

Why this matters

Downstream tooling that persists or hashes the concept set expression treats the response as content. A pure reordering is indistinguishable from a real edit:

  • the serialized JSON on disk changes,
  • its content hash changes, so change-detection reports a modification that did not happen,
  • study repositories under version control accumulate spurious diffs and uncommitted changes.

This is reported from downstream use in OHDSI/Picard, which imports concept sets and cohort definitions from ATLAS into a study repository and uses content hashing to decide whether an input has actually changed. Any reproducible-study tooling that round-trips concept sets through WebAPI has the same exposure, and the same applies to cohort definitions once a re-saved cohort picks up a reordered embedded concept set expression.

Endpoints involved
  • GET /conceptset/{id}/expression
  • GET /conceptset/{id}/expression/{sourceKey}
  • GET /conceptset/{id}/version/{version}/expression
  • GET /conceptset/{id}/items
Where this appears to originate

Reading the source on master (a9720e7), the order of expression.items is simply the row order the database happens to return, and nothing in the chain imposes a deterministic order:

  1. The expression is assembled by iterating repositoryItems in list order:
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java#L336-L345

  2. repositoryItems is populated from getConceptSetItems(id):
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java#L296-L300
    which delegates to the repository:
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java#L202-L206

  3. findAllByConceptSetId is a Spring Data derived query with no OrderBy clause and no Sort argument, so the generated SQL has no ORDER BY and the row order is unspecified:
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/java/org/ohdsi/webapi/conceptset/ConceptSetItemRepository.java#L26

    This is the platform-independent core of the report: without an ORDER BY, the row order is formally unspecified on any database, and a client cannot rely on it being stable between two identical requests.

  4. concept_set_item carries only a primary key on concept_set_item_id; I could not find any index on concept_set_id in the migrations, so the lookup is a full scan filtered on concept_set_id:
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/resources/db/migration/postgresql/V1.0.1.0__conceptsets.sql#L12-L19
    https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/resources/db/migration/postgresql/V2.3.0.20180412000001__constraints.sql#L5

    A candidate mechanism, if the affected instance runs on PostgreSQL. I have not confirmed which database platform this particular WebAPI instance uses, so I offer this conditionally rather than as a diagnosis. On PostgreSQL, an unordered sequential scan is exactly the situation where identical queries can legitimately return rows in different orders with no data change at all — synchronize_seqscans is on by default, so a scan may start at an arbitrary block and wrap around, and a parallel sequential scan interleaves worker output nondeterministically. A plan change, a VACUUM FULL/CLUSTER, or restoring the OHDSI schema from a dump will also change heap order. Whether that specific mechanism applies would also depend on how large concept_set_item is relative to shared_buffers on the affected instance — is that something you would expect to be significant on a typical deployment? On other supported platforms the missing ORDER BY still leaves the order unspecified, though the practical trigger would differ. Happy to confirm the platform and table size if that helps narrow it down.

  5. Nothing downstream restores an order: circe's ConceptSetExpression.items is a plain ConceptSetItem[] and faithfully serializes whatever order it is handed, so this does not look like a circe-be or ATLAS issue.

One related observation: the same unordered query is used when a concept set version snapshot is created, so an arbitrary row order is frozen into concept_set_version.asset_json:
https://github.com/OHDSI/WebAPI/blob/a9720e7bc0bec454d3363a61999b10677e448af6/src/main/java/org/ohdsi/webapi/conceptset/converter/ConceptSetToConceptSetVersionConverter.java#L26

There is precedent for caring about deterministic serialization here — OHDSI/circe-be#48 explicitly pinned property order for Concept serialization for the same class of reason.

Possible fix — question for maintainers

Would you be open to giving these reads a deterministic order? A couple of options, and I'd defer to you on which fits best:

  • Add an explicit ordering to the repository method, e.g. findAllByConceptSetIdOrderByConceptIdAsc, or pass a Sort. Ordering by concept_id is stable across databases and survives a delete-and-reinsert of the items on save, which concept_set_item_id would not.
  • Or sort expressionItems in ConceptSetService.getConceptSetExpression before assigning expression.items, which localizes the change to the expression endpoints.

An accompanying index on concept_set_item(concept_set_id) would probably be worthwhile regardless, both for the sort and for the lookup itself.

A related question: is item order intended to be semantically meaningful anywhere (i.e. does anything rely on the current insertion order coming back), or is a concept set expression conceptually an unordered set? If it is unordered, normalizing the order on read seems safe and would make the endpoint's output content-addressable.

Happy to supply more

We can attach an example concept set that exhibits the reordering, along with the responses from two consecutive identical requests, so you have something concrete to reproduce against — I'll follow up with that below. If there is anything else that would be more useful to see, just say. We're also glad to test a fix against our downstream usage.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with ConceptSetItemRepository.java and the getConceptSetItems and getConceptSetExpression entry points in ConceptSetService.java. Trace the four listed concept-set reads and the ConceptSetToConceptSetVersionConverter.java path, then check how existing tests cover their serialized item order. Done means the affected responses and version snapshots have a documented, deterministic order without changing their contents.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, spring
Domain
api, backend, database
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
67/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.