Skip to content

Setting trackerFileExtension to the instrument name fails with a SQL syntax error, or is silently ignored #311

Closed

Description

@ZohebShaikh

Summary

configure skips a trackerFileExtension that equals the instrument name (src/db_service.rs, update_instrument):

if let Some(ext) = &self.tracker_file_extension {
    if ext != &self.name {
        // extension defaults to instrument name
        fields.push("tracker_file_extension=");
        fields.push_bind_unseparated(ext);
    }
}

This causes two problems:

  1. SQL syntax error. When the extension is the only field being updated, the is_empty() check passes (the extension is Some), but no field is added to the SET list. The query becomes UPDATE instrument SET WHERE name = ? RETURNING *, and the mutation returns
    error returned from database: (code: 1) near "WHERE": syntax error.
  2. The extension can't be reset to the default. Once an extension has been set to something else, setting it back to the instrument name is silently dropped. If another field is updated in the same call, the mutation succeeds and returns the old extension unchanged.

A related inconsistency: creating an instrument with trackerFileExtension equal to its name stores it explicitly. The check only applies to updates. So two instances that behave identically can report null and "b01-1" for the same instrument.

Steps to reproduce (v1.1.0, also in v1.0.1)

git checkout v1.1.0
cargo build
./target/debug/numtracker serve --db repro.db --port 8123

Run each mutation against http://localhost:8123/graphql, for example in graphiql:

1. Create an instrument without an extension. This works and returns trackerFileExtension: null.

mutation {
  configure(instrument: "b01-1", config: {
    directory: "/tmp/{instrument}/data/{year}/{visit}"
    scan: "{subdirectory}/{instrument}-{scan_number}"
    detector: "{subdirectory}/{instrument}-{detector}-{scan_number}"
  }) { instrument trackerFileExtension }
}

2. Set the extension to the instrument name. ❌ This returns the SQL syntax error.

mutation {
  configure(instrument: "b01-1", config: { trackerFileExtension: "b01-1" }) { instrument trackerFileExtension }
}
{"data":null,"errors":[{"message":"error returned from database: (code: 1) near \"WHERE\": syntax error","path":["configure"]}]}

3. Set the extension to something else. This works and returns trackerFileExtension: "other".

mutation {
  configure(instrument: "b01-1", config: { trackerFileExtension: "other" }) { instrument trackerFileExtension }
}

4. Try to set it back to the instrument name, with another field. ❌ This succeeds, but the extension is still "other".

mutation {
  configure(instrument: "b01-1", config: { trackerFileExtension: "b01-1", scanNumber: 5 }) { instrument trackerFileExtension }
}
{"data":{"configure":{"instrument":"b01-1","trackerFileExtension":"other"}}}

5. Create a different instrument with the extension equal to its name. This stores it explicitly (trackerFileExtension: "i22"), which isn't consistent with step 2.

mutation {
  configure(instrument: "i22", config: {
    directory: "/tmp/{instrument}/data/{year}/{visit}"
    scan: "{subdirectory}/{instrument}-{scan_number}"
    detector: "{subdirectory}/{instrument}-{detector}-{scan_number}"
    trackerFileExtension: "i22"
  }) { instrument trackerFileExtension }
}

Expected

  • Setting the extension to the instrument name updates it. In the database, that means setting tracker_file_extension to NULL (the default) or to the name; either works, since NumTracker::for_instrument falls back to the instrument name with ext.unwrap_or(bl).
  • An update that ends up with no fields to set returns the current configuration, as is_empty() already does, instead of running an invalid UPDATE.

Suggested fix

In update_instrument, write NULL when the extension equals the name instead of skipping it:

if let Some(ext) = &self.tracker_file_extension {
    fields.push("tracker_file_extension=");
    // extension defaults to instrument name, so store NULL rather than duplicating it
    fields.push_bind_unseparated((ext != &self.name).then_some(ext));
}

This fixes all three problems: no empty SET, the extension can be reset, and the default is stored the same way every time. insert_new could normalise the same way for consistency.

How this was found

An integration test comparing the configurations query on prod and staging reported b01-1 as null on one and "b01-1" on the other. Trying to align them with the configure mutation hit the syntax error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions