iTwin
Misleading error thrown by getElementProps
Describe the bug iModel.Elements.getElementProps throws 'Element not found' regardless of the cause of the error.
To Reproduce
const regionId = "0x000001" // a valid id for a region element in the iModel
const badId = {id: regionId} as unknown as Id64String;
const element = iModel.elements.getElementProps<SectionDrawingLocationProps>({ id: badId, wantGeometry: true });
other variations throw the same error:
//const spatialRegion = iModel.elements.getElementProps<SectionDrawingLocationProps>({ id: badId , wantGeometry: true });
//const spatialRegion = iModel.elements.getElementProps<SectionDrawingLocationProps>(badId );
//const spatialRegion = iModel.elements.tryGetElementProps(badId );
//const spatialRegion = iModel.elements.tryGetElement(badId );
Expected behavior Throw a more meaningful error.
Where did you get that Id from? "0x000001" is not a valid element Id - leading zeroes are not permitted. "0x1" is a valid element Id, but it's always the root subject, not a region element.
The id is not the point of the issue.... its just a string. The point is that the argument is not a valid type for the function.
The id is not the point of the issue.... its just a string. The point is that the argument is not a valid type for the function.
The issue claims we throw "not found" regardless of the cause of the error. That it not true. We throw "not found" if you give us an identifier like "0x000001" that doesn't identify any element. If you just made up "0x000001" for this issue, then please provide an actual case that produces the problem instead.
I think you are taking issue with the fact that getElementProps calls tryGetElementProps which returns undefined if any exception occurs. getElementProps throws "not found" if tryGetElementProps returned undefined. Any other error that occurs inside tryGetElementProps is ignored.
If so, I agree, that's kinda dumb. It should work the other way: the guts of tryGetElementProps belong inside getElementProps with no try-catch; tryGetElementProps should call getElementProps inside a try-catch and return undefined if any exception is thrown.
However, I'm not confident that apps have not come to rely on the current behavior. I'm not sure how common it is for any other kind of error to occur besides "not found" when attempting to load an element. And I still want to know what you would consider a more meaningful error when you give us an identifier that doesn't correlate to any element.
I also just attempted to recreate this with a real id to verify:
const regionId = "0x23";
const badId = { id: regionId } as unknown as Id64String;
Even when casting a valid ElementLoadProps object into a string parameter, native (and thus tryGetElement()) will return a valid object if it exists, since the native function can also accept ElementLoadProps. In this case returning a BisCore:DocumentPartition with id 0x23.
That being said, if I do force in an object that makes no sense
const regionId = "0x23";
const badId = { foobar: regionId } as unknown as Id64String;
We get undefined back from native (not an error), and then generate this error, which does show that everything is undefined, but is not as straight forward as "incorrect type". "Element={id: undefined federationGuid: undefined, code={spec: undefined, scope: undefined, value: undefined}}"
We could add logic to verify that the parameter is a valid structure and return an error if native returns undefined and its not, but that would require someone to be deliberately incorrectly casting in the first place in order to violate the strong typing of the parameter in the first place. I think checking for that is a bit redundant.
I do agree with @pmconne that tryGetElementProps and getElementProps are confusing. This goes the same with getModelProps and tryGetModelProps. Both swallow errors returned from Native and generate new, less informative errors.
Following up on this, do we want to create a separate issue for refactoring tryGetElementProps?
I think in the case that @Josh-Schifter has described, where you pass a valid ElementLoadProps as a string, we should expect a "element not found" error.
No lets not create another issue for tryGetElementProps; we can handle them in the same pr and link to the same issue (this)
We need some documented standards here and guidance for how to handle failures to load elements because the handler detects an error. That said I don't want to hold up this PR until we have that, I suggest we create an issue to track and then resolve this comment.
from https://github.com/iTwin/itwinjs-core/pull/8570#discussion_r2380194066
Working on this issue would be a good opportunity to document the standards and best practices here for handling failures when trying to load elements
cc @ColinKerr @Alfonso-Martello
Would it be considered a breaking change to rework the errors returned by getElement?
Would it be considered a breaking change to rework the errors returned by getElement?
No i dont think so; as long as api signature doesnt change it should be ok
Would it be considered a breaking change to rework the errors returned by getElement?
No i dont think so; as long as api signature doesnt change it should be ok
If something was OK but is now an error we have to consider the change very carefully. On the other hand if something was silently failing or inserting invalid stuff then turning that into an explicit error is generally OK.
