Fixed: Improved exception message on failed schema import #8796
Conversation
|
|
|
I suggest you instead use this opportunity to introduce a new ITwinError interface for schema related errors instead of propagation the ErrorNumber insanity. |
| const schemaPart = `Schema(s): ${schemaSummary}.`; | ||
| const message = nativeMessage ? `${base} ${schemaPart} ${nativeMessage}` : `${base} ${schemaPart}`; | ||
|
|
||
| Logger.logError(loggerCategory, message, () => ({ |
There was a problem hiding this comment.
Do not log errors when an exception is thrown. Log them where it is caught.
| Logger.logError(loggerCategory, `Failed to import schema from '${pathName}'.`); | ||
| throw new IModelError(status, `Failed to import schema from '${pathName}'.`); | ||
| const errMsg = `Schema import failed while importing/validating schema. Schema: ${pathName}. (status ${status})`; | ||
| Logger.logError(loggerCategory, errMsg, () => ({ pathName, status })); |
There was a problem hiding this comment.
| Logger.logError(loggerCategory, errMsg, () => ({ pathName, status })); |
When you log, there's no need for the 3rd argument to be a function. Instead pass the metadata object.
|
|
||
| if (!fs.existsSync(pathName)) { | ||
| const errMsg = `Schema import failed while loading schema file. Schema: ${pathName}. File does not exist.`; | ||
| Logger.logError(loggerCategory, errMsg, () => ({ pathName })); |
There was a problem hiding this comment.
Don't log here. Callers should log when they catch the exception.
| fs.accessSync(pathName, fs.constants.R_OK); | ||
| } catch (err) { | ||
| const errMsg = `Schema import failed while loading schema file. Schema: ${pathName}. ${(err as Error).message}`; | ||
| Logger.logError(loggerCategory, errMsg, () => ({ pathName })); |
|
|
||
| const loggerCategory: string = BackendLoggerCategory.IModelDb; | ||
|
|
||
| function summarizeSchemaSources(schemaSources?: readonly string[]): string { |
There was a problem hiding this comment.
It looks to me like you need to move all the schema related code to a new file. Polluting IModelDb.ts isn't a good idea.
There was a problem hiding this comment.
Thanks for your feedback, I will make the suggested changes and update the pr shortly
3 participants
Fixes #8656
Improves exception handling during schema import operations to provide clearer and more actionable error messages.
Summary of Changes
1. ECDb Error Improvements (core/backend/src/ECDb.ts):
Added pre-validation for schema file existence (fs.existsSync / fs.accessSync)
Throw structured IModelStatus.FileNotFound errors including file path context
Wrapped importSchema native call with contextual exception handling
2. IModelDb Enhancements (core/backend/src/IModelDb.ts):
Introduced createSchemaImportError helper for consistent error messaging
Pre-validation for imported schema file paths
Improved context propagation for:
3. Regression Fix (core/backend/src/test/hubaccess/Rebase.test.ts):
Corrected incorrect usage of importSchemas in existing test
Now writes schema XML to a temp file before import
4. Added New Tests:
-Covers:
Validation:
1. Automated:
Manual:
📸 Screenshot of test results