Changes for bulk upload #506
Conversation
|
@jftl6y Will you please parameterize the SimpleChat FQDN in the Powershell Script? Make it a mandatory parameter. @paullizer and I will finish reviewing this PR today I believe. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds bulk document upload functionality to SimpleChat via external API endpoints, introducing support for both public and group workspace document uploads through a new PowerShell-based utility called PSLoader.
Key Changes
- Added new external API routes for group document management (upload, list, delete) using OAuth Bearer token authentication
- Created PSLoader PowerShell utility with comprehensive documentation for bulk document uploads
- Fixed spelling in app.py comment ("Extenral" → "External")
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 32 comments.
| File | Description |
|---|---|
application/single_app/route_external_group_documents.py |
New file implementing external API routes for group workspace document operations (upload, list, delete) with OAuth authentication |
application/single_app/app.py |
Added imports and route registration for group documents API, fixed typo in comment |
application/external_apps/PSLoader/README.md |
Comprehensive documentation for PSLoader utility including setup, authentication, usage examples, and troubleshooting |
application/external_apps/PSLoader/PSLoader.ps1 |
PowerShell script implementing bulk document upload with OAuth client credentials flow and multi-cloud support |
| - `.pdf`, `.docx`, `.txt`, `.md`, `.html` | ||
|
|
||
| ### Spreadsheets | ||
| - `.xsl`, `.xslx`, `.csv` |
There was a problem hiding this comment.
Spelling error: .xsl should be .xls (Excel spreadsheet file extension). The correct extension is .xls, not .xsl (which is for XSL Transformation files).
| - `.xsl`, `.xslx`, `.csv` | |
| - `.xls`, `.xlsx`, `.csv` |
| write-host "Retrieved access token. Beginning file upload process..." | ||
|
|
||
| $files = @(Get-ChildItem -Path $uploadDirectory -File -Recurse -Include *.pdf, *.docx, *.pptx, *.txt, *.md, *.json, *.html, ` | ||
| *.xsl, *.xslx, *.csv, *.jpg, *.jpeg, *.png, *.bmp, *.tiff, *.tif, *.heif, ` |
There was a problem hiding this comment.
Spelling error: .xsl should be .xls (Excel spreadsheet file extension). The correct extension is .xls, not .xsl (which is for XSL Transformation files).
| *.xsl, *.xslx, *.csv, *.jpg, *.jpeg, *.png, *.bmp, *.tiff, *.tif, *.heif, ` | |
| *.xls, *.xlsx, *.csv, *.jpg, *.jpeg, *.png, *.bmp, *.tiff, *.tif, *.heif, ` |
|
|
||
| ```powershell | ||
| # Current (line 119): | ||
| $uploadResponse = Invoke-RestMethod -Method Post -Uri "https://your-simplechat-api-endpoint/api/external/public/documents/upload" -Headers $headers -Form $form |
There was a problem hiding this comment.
Inconsistent API endpoint path: Line 262 shows the endpoint as https://your-simplechat-api-endpoint/api/external/public/documents/upload with /api/ prefix, but line 265 shows it as https://your-app.azurewebsites.net/external/public_documents/upload without /api/ prefix. The correct path (based on the route definition in the code) should not include /api/ prefix. Please correct line 262 to match the actual route pattern.
| $uploadResponse = Invoke-RestMethod -Method Post -Uri "https://your-simplechat-api-endpoint/api/external/public/documents/upload" -Headers $headers -Form $form | |
| $uploadResponse = Invoke-RestMethod -Method Post -Uri "https://your-simplechat-api-endpoint/external/public_documents/upload" -Headers $headers -Form $form |
|
|
||
| $uploadResponse = Invoke-RestMethod ` | ||
| -Method Post ` | ||
| -Uri "https://your-simplechat-api-endpoint/api/external/public/documents/upload" ` |
There was a problem hiding this comment.
Incorrect API endpoint path: The endpoint path includes /api/ prefix (/api/external/public/documents/upload), but based on the route definitions in the code, the correct path should be /external/public_documents/upload (without /api/ and with underscore in public_documents). Please correct this to match the actual route pattern.
| -Uri "https://your-simplechat-api-endpoint/api/external/public/documents/upload" ` | |
| -Uri "https://your-simplechat-api-endpoint/external/public_documents/upload" ` |
| """ | ||
| user_id = request.args.get('user_id') | ||
| active_workspace_id = request.args.get('active_workspace_id') # This is group_id | ||
|
|
There was a problem hiding this comment.
Missing input validation: The user_id and active_workspace_id parameters are not validated for presence before being used in database queries. If these are None, it could lead to unexpected query behavior. Consider adding validation checks similar to the upload endpoint (lines 32-36).
| if not user_id or not active_workspace_id: | |
| return jsonify({'error': 'user_id and active_workspace_id required'}), 400 |
| # route_external_group_documents.py | ||
|
|
||
| from config import * | ||
| from functions_authentication import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module functions_authentication does not define 'all'.
| from functions_authentication import * | |
| from functions_authentication import accesstoken_required |
| except Exception as e: | ||
| upload_errors.append(f"Failed to save temporary file for {original_filename}: {e}") | ||
| if temp_file_path and os.path.exists(temp_file_path): | ||
| os.remove(temp_file_path) |
There was a problem hiding this comment.
This statement is unreachable.
|
@jftl6y Review the copilot recommendations. I resolved out ones that are ignorable for us. The others, especially the ones around authorization, need to be reviewed and possibly implemented. Let us know if you have any questions (one of them covers the hard-coded endpoint in the PS script as well I mentioned earlier.). |
2 participants
Added fixes to allow public/group document bulk upload via API. Added a PowerShell version of the BulkLoader utility with README.md with more detail on configuring the service principals.