[RTI][TOOL] Unique IDs generation for RTI Requests#153
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the unique identifier for RTI Requests from UUIDs to auto-incrementing integers across the database schema, backend services, API routers, tests, and frontend components. The review feedback highlights three key issues: first, defining the primary key id in SQLModel without a default value will cause validation errors during instantiation; second, calling self.session.flush() introduces a transaction that is not rolled back when custom exceptions are raised; and third, parsing route parameters with Number(id) can yield NaN for non-numeric URLs, which should be safely handled using parseInt with a fallback.
| rti_template_id=request_data.rti_template_id | ||
| ) | ||
| self.session.add(rti_request) | ||
| self.session.flush() |
There was a problem hiding this comment.
With the introduction of self.session.flush() before the file upload and status history creation, a database transaction is now active. If any subsequent step fails (such as the file upload failing or the CREATED status not being found in the database), an InternalServerException or other custom exception is raised.
However, the exception handling block at the end of this function catches these custom exceptions and directly re-raises them:
except (BadRequestException, NotFoundException, ConflictException, InternalServerException):
raiseThis bypasses the except Exception as e: block, meaning self.session.rollback() is never called. This leaves the database transaction unrolled-back and can lead to leaked resources, lock contention, or partial/corrupted data being committed later.
To fix this, restructure the exception handling at the end of the function to catch all exceptions, perform the rollback and file cleanup, and then re-raise or map them accordingly.
There was a problem hiding this comment.
Noted! Not this exception raising handled inside the the catch block and re-raise the exception based on the exception type
|
Add ability in the frontend to search by the id of the rti request. Currently it only searches by title. |
|
When you view an rti request, the id should be shown on the page. |
|
Remove Active flag from the latest state in each rti requests |
|
Change "TARGET ENTITY" to "RECEIVER" in the rti_request page. |
| target_id = UUID(request_id) if isinstance(request_id, str) else request_id | ||
| except ValueError: | ||
| raise BadRequestException(f"Invalid UUID format: {request_id}") | ||
| target_id = request_id |
| ): | ||
| request = RTIRequestUpdateRequest( | ||
| id=id or str(uuid.uuid4()), | ||
| id=id or 99999, |
There was a problem hiding this comment.
| id=id or 99999, | |
| id=id or 999999, |
| request = RTIRequestUpdateRequest(**fields) | ||
| if not id: | ||
| request.id = str(uuid.uuid4()) # Ensure ID is always set for service fetch | ||
| request.id = 99999 # Ensure ID is always set for service fetch |
There was a problem hiding this comment.
| request.id = 99999 # Ensure ID is always set for service fetch | |
| request.id = 999999 # Ensure ID is always set for service fetch |
| @pytest.mark.asyncio | ||
| async def test_get_rti_request_histories_not_found(rti_request_db, make_file_service): | ||
| """NotFoundException raised when RTI Request ID doesn't exist.""" | ||
| service = RTIRequestHistoryService(session=rti_request_db, file_service=make_file_service()) | ||
|
|
||
| with pytest.raises(NotFoundException) as exc: | ||
| service.get_rti_request_histories_by_id(rti_request_id=uuid.uuid4()) | ||
| assert "not found" in str(exc.value) |
|
|
||
| with pytest.raises(NotFoundException) as exc: | ||
| await service.create_rti_request_history(rti_request_id=uuid.uuid4(), request_data=request) | ||
| await service.create_rti_request_history(rti_request_id=99999, request_data=request) |
There was a problem hiding this comment.
| await service.create_rti_request_history(rti_request_id=99999, request_data=request) | |
| await service.create_rti_request_history(rti_request_id=999999, request_data=request) |
|
|
||
| with pytest.raises(NotFoundException) as exc: | ||
| service.get_rti_request_by_id(request_id=uuid.uuid4()) | ||
| service.get_rti_request_by_id(request_id=99999) |
There was a problem hiding this comment.
| service.get_rti_request_by_id(request_id=99999) | |
| service.get_rti_request_by_id(request_id=999999) |
|
|
||
| with pytest.raises(InternalServerException) as exc: | ||
| service.get_rti_request_by_id(request_id=uuid.uuid4()) | ||
| service.get_rti_request_by_id(request_id=99999) |
There was a problem hiding this comment.
| service.get_rti_request_by_id(request_id=99999) | |
| service.get_rti_request_by_id(request_id=999999) |
2 participants
Closes: #151