iTwinJs Core using errors for basic code flow

[ColinKerr]

I see three main code flows where we are throwing errors from some methods and then branching on those errors inside catch block.

1. The first place is here https://github.com/iTwin/itwinjs-core/blob/soham-bentley/debugging-exceptions/core/backend/src/IModelDb.ts#L1286-L1294

Here as we can see in the catch block if the error has the error number of IModelStatus.NotFound we branch on it and load meta data for this class first and then try to get the class again.
Upon inspection I found that
https://github.com/iTwin/itwinjs-core/blob/soham-bentley/debugging-exceptions/core/backend/src/ClassRegistry.ts#L309-L312

getClass can internally throw error from generateClass

https://github.com/iTwin/itwinjs-core/blob/soham-bentley/debugging-exceptions/core/backend/src/ClassRegistry.ts#L281-L292

generateClass can throw errors from the following three lines

itwinjs-core/core/backend/src/ClassRegistry.ts

Line 285 in c760c18

throw this.makeMetaDataNotFoundError(classFullName);

itwinjs-core/core/backend/src/ClassRegistry.ts

Line 289 in c760c18

this.getClass(metadata.baseClasses[0], iModel);

itwinjs-core/core/backend/src/ClassRegistry.ts

Line 292 in c760c18

return this.generateClassForEntity(metadata, iModel);

Among these three errors , when the metadata for that particular className comes as undefined we try to handle it manually inside the catch block (https://github.com/iTwin/itwinjs-core/blob/soham-bentley/debugging-exceptions/core/backend/src/IModelDb.ts#L1286-L1294) by loading the metadata and trying again...the second time the metadata will surely not be undefined.
Upon close inspection we can understand that other than this specific error, the other errors which are thrown from generateClass will be thrown the second time as well when we call getClass the second time from the catch block and therefore the other errors are treated and respected as errors and bubbled up the chain.

This complex workflow for simple metadata loading leads to many false positive errors. I believe there is a more and simple and elegant way to handle this.

2. resolveElementKey method of IModelDb.ts itself throws errors and also calls resolveInstanceKey method of nativeDb(https://github.com/iTwin/itwinjs-core/blob/soham-bentley/debugging-exceptions/core/backend/src/IModelDb.ts#L2186-L2215) which also internally throws errors on the native side.

resolveElementKey method is being referenced by tryGetElementProps method (

itwinjs-core/core/backend/src/IModelDb.ts

Line 2240 in c760c18

const instanceKey = this.resolveElementKey(props);

)
AND
by queryElementIdByCode method (

itwinjs-core/core/backend/src/IModelDb.ts

Line 2315 in c760c18

const elementKey = this.resolveElementKey(codeToQuery);

)

Here also in both the methods tryGetElementProps and queryElementIdByCode we capture the erros in catch block and branch on it..but the branching logic is quite different in case of both methods

3. Similarly resolveInstanceKey method of nativeDb is also referenced by resolveModelKey method.(

   itwinjs-core/core/backend/src/IModelDb.ts

   Line 1990 in c760c18

   private resolveModelKey(modelIdArg: ModelLoadProps): IModelJsNative.ResolveInstanceKeyResult {

   ) which is in turn referenced by tryGetModelProps method (

   itwinjs-core/core/backend/src/IModelDb.ts

   Line 1917 in c760c18

   public tryGetModelProps<T extends ModelProps>(id: Id64String): T | undefined {

   )
   Here also we do branching in catch block and return undefined.

[ColinKerr]

From @soham-bentley

For point 1 I have a fix which slightly changes the behavior of generateClass method thereby slightly changing the behavior of getClass method
It is included in the commit
4e26a18

Just replacing manual lookup of metadata inside the generateClass method with the getMetaData method solves the complex workflow as getMetaData method internally handles loading incase metadata is not found the first time

itwinjs-core/core/backend/src/IModelDb.ts

Lines 1327 to 1339 in c760c18

	public getMetaData(classFullName: string): EntityMetaData {
	// eslint-disable-next-line @typescript-eslint/no-deprecated
	let metadata = this.classMetaDataRegistry.find(classFullName);
	if (metadata === undefined) {
	// eslint-disable-next-line @typescript-eslint/no-deprecated
	this.loadMetaData(classFullName);
	// eslint-disable-next-line @typescript-eslint/no-deprecated
	metadata = this.classMetaDataRegistry.find(classFullName);
	if (metadata === undefined)
	throw ClassRegistry.makeMetaDataNotFoundError(classFullName); // do not log
	}
	return metadata;
	}

For points 2 and 3 what should be the expected behavior?
Should we stop throwing errors from native completely and just always return undefined instead?
Even if we stop throwing errors from native side we will still need to get rid of all errors on the core side as well...to get completely rid of catch level branching

[ColinKerr]

From @rschili

In the other story I believe I linked and mentioned several related issues.
We can bonk our heads over it all we want, the bottom line is we should decide for a line of action and stick with it.
In the past there have been clashes between people advocating error/return codes and others advocating throwing. But this lead us to move from return codes to errors in cases which are considered normal code flow.
Many modern languages (go, rust, typescript) use methods that can return their value along with some status information, it's as simple as just returning a union/optional.
I don't think many people will argue against this being a right move for normal code flow.

We should make that decision with everybody involved and document it somewhere in our CONTRIBUTING.md file, and then whenever we hit such spots, we should just slowly refactor towards the desired goal, of course keeping in mind that we may not be able to break some APIs.

So, first step: Make a decision with a large enough audience, give people a chance to object, and then document the decision.