Skip to content

Fix create_session API endpoint and perform housekeeping on murfey.server.api.session_info - #906

Open
tieneupin wants to merge 14 commits into
mainfrom
create-session-fix
Open

tieneupin wants to merge 14 commits into
mainfrom
create-session-fix

Conversation

@tieneupin

@tieneupin tieneupin commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

NOTE: This PR will need to be deployed alongside DiamondLightSource/murfey-frontend#88

The create_session FastAPI endpoint previously parsed the visit (e.g. 'cm12345-6') and name (an optional long-form description) directly from the URL path. This is unsafe, as the presence of characters such as '/' in the parametrers will break the URL construction logic.

This PR fixes that by passing the visit and name as part of the JSON data payload instead. The API endpoint path was also changed now that visit and name are no longer needed as URL path parts.

As part of housekeeping, the way in which the Murfey database tables were imported and called was also standardised. Instead of importing the tables directly, we use import murfey.util.db as MurfeyDB and called tables from MurfeyDB. Type hints were also replaced with more modern variants.

A bug was also identified when seeding the database fixtures upon startup. For integer primary keys, if a value is manually provided as part of the initial insert, it does not update PostGreSQL's internal counter. Subsequent inserts where the primary key is allow to auto-increment will then attempt to insert into that database with the previous primary key, causing the insert to fail. By removing the primary key from the initial insert in the database fixture, the internal PostgreSQL counter is now updated correctly.

@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.48%. Comparing base (cc9def2) to head (1f23e02).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
+ Coverage   55.42%   55.48%   +0.06%     
==========================================
  Files         103      103              
  Lines       11546    11547       +1     
  Branches     1541     1541              
==========================================
+ Hits         6399     6407       +8     
+ Misses       4800     4793       -7     
  Partials      347      347              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tieneupin tieneupin changed the title Create session fix Fix create_session API endpoint and perform housekeeping on murfey.server.api.session_info Oct 1, 2026
@tieneupin
tieneupin marked this pull request as ready for review October 1, 2026 14:38
@tieneupin tieneupin self-assigned this Oct 1, 2026
@tieneupin tieneupin added bug Something isn't working server Relates to the server component labels Oct 1, 2026
Comment thread src/murfey/server/api/session_info.py Outdated
name=name,
visit=visit,
session = MurfeyDB.Session(
name=session_info.name,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

name definitely needs sanitising - I'd suggest stripping out anything except standard alphanumeric and _ or -

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've tacked on the sanitise() function to both of them.

Comment thread src/murfey/server/api/session_info.py Outdated
visit=visit,
session = MurfeyDB.Session(
name=session_info.name,
visit=session_info.visit,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably also worth sanitising the visit in the same way

This branch has not been deployed

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

Labels

bug Something isn't working server Relates to the server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants