-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
If ConflictingLocksError is thrown on the backend during an IPC call rethrow it on the frontend #6909
base: master
Are you sure you want to change the base?
Conversation
…when backend throws it during IPC call. Definitely needs work.
I think this approach is heading in the wrong direction. We should not create any subclasses of Error, and certainly no new ones. Testing for BTW, I realize terminology is difficult, but "lock conflict" is confusing. Conflicts are what you get if you don't use locks. The error here must be "lock is in use", right? |
Since the conflictinglockserror passes IModelHubStatus.LockOwnedByAnotherBriefcase as its errorNumber, the message ultimately contains "Lock is owned by another briefcase". Im guessing you meant instanceof? The imodels-clients repo will start throwing this error class during acquireLocks, so I assume that the instanceof check would pass in that case? I agree we can't just keep creating subclasses of error, but I assumed switching over to something more uniform would have to be a breaking change for 5.0. I've been thinking about this problem in the context of a singular ITwinError interface as you suggested as well, in the workitem that I linked in the description. |
Maybe "AlreadyLocked" would be better than "ConflictingLock"? |
Yes, that error makes more sense to me. Locks are either available or "in use" (the distinction is that shared locks block exclusive locks). I'd suggest "LockInUse" or "LockNotAvailable". Your suggestion is fine too, although the lock may be a shared lock, some may find that confusing. The problem is that Conflicts come during Merge operations, that's why I was confused when I saw this PR. |
It would be breaking to attempt to change existing subclasses of Error, but not if you're making up a new class. I suggest this be the first use of ITwinError and we never support Error subclasses "spanning" processes.
|
This will only be useful if we give the "Moniker" of the lock holder (so the localized message will be "Element is in use by @joesmith"), right? Hopefully the IMS team and IModelHub are working on that? |
One last point, Locks lock Elements. When we refer to "locks" in error messages, that will confuse users - they only know about Elements. From their perspective the Element is locked. [N.B. there is some mystery about locking Models - we really obtain the lock on its modeled element. Also the "schema lock" is really a lock on the root subject Element.] |
I agree with most of @kabentley's sentiments. I think this discussion belongs on the issue, but he lacks access to it. The proposed error interface looks like this: export interface ITwinError {
namespace: string; // Localization namespace
errorKey: string; // unique key for error, within namespace
message: string; // explanation of what went wrong. (Should provide some format for localization, like ITwinLocalization.getLocalizedKeys but the opposite way)
metadata?: LoggingMetaData;
} This is great for being able to present meaningful information to the user. Software can recognize the unique combination of namespace+errorKey and react to specific errors if it needs to. It will not, however, have access to additional details like the list of unavailable locks.
If the only error details available are encoded into the localizable string then all of the actionable details must be encoded into it. An operation can require multiple locks held by multiple different people, for example. That could make for a long, ugly user-facing error message. Presumably ITwinError must include an object containing variables to replace placeholders in the localized message. e.g., "Element is in use by ${lockOwner}". Only the frontend can do the substitution because only it knows the user's locale. Relying on namespace+errorKey as the "error code" may prove fragile. For example, we may need two different errorKeys for the "lock unavailable" error, depending on whether one or multiple locks are not held. Or, we may decide in the future to rephrase an error such that it requires different formatting (e.g., takes a different number of variables), necessitating a new errorKey to avoid breaking RPC-based frontends that are using an older version of the localizable error message. Suddenly my If we want software to be able to do anything with these errors, then we would effectively be making the localization keys an untracked part of our public API. |
I'll share my thoughts from an email thread that requested I feel like I want to localize errors on the Frontend. It is because Frontend is aware of the context - error messaging depends on the workflow the user was attempting. I mean, the same
Translating errors on the Backend (returning a key) assumes that an error is always directly translated to the same user facing text - I don't think that is true in general. That will end up feeling "dull" and lack flexibility (limiting UX). |
From what I understood, since the ConflictingLocks interface is a part of the error message, it would be used to see the userid of who owns the briefcaseId and and then query the hub's 'users' endpoint to get information about the user. My suggestion on the issue is to add another field 'extraData' whose type is driven by the namespace + errorKey (Note I only know how to do this if we combined namespace and errorkey). In the ConflictingLocks case, extraData would be of type ConflictingLocks[]. See playground. This would give consumers compile time errors if we decided to change the type ConflictingLocks for some reason. This extraData would allow consumers to specialize the message they show to users if they so desire. If they saw no need to, they'd be able to use our fallback message. I don't think this solves the problem Paul brings up of our messages potentially needing to change formats, but maybe we'll see a need for changing message formats less if consumers can make their own message using the data provided in the 'extraData' field? |
This will encourage bad patterns of not caring and doing proper localization, because the responsibility won't be on the app developers, e.g. "it will be handled by the platform". It seems awkward to me that the Backend needs to deal with localization. It doesn't even deliver the localization files, "how will it know" what keys exist? The Backend has no idea of what is happening on the Frontend - how can it provide a UX friendly error message? Also, what about application defined errors? Can we get a mechanism to pass those to the Frontend? |
Isn't providing this extraData putting it on the app developers to create a message as they see fit? Or do you mean having a message to fallback to will encourage bad patterns? I figured it might be wasteful to force every single app to implement their own error message if we can provide a default that works just fine. I imagined app developers noticing a message they don't like and iteratively making custom ones. |
I think there is some confusion about my comments. There are 3 separate topics here:
I can't see the rest of the proposal for ITwinError, but the comment above about |
In such case, how would we communicate to application developers what error fields to expect and of what type? If we rely on applications discovering Error fields themselves, it sounds like we could not really change them once we add them, since it is possible they added code that relies on those fields and they will not get any compiler errors like they would if we changed names/property types on a class. |
Could you give an example of what the message field would look like? When you initially proposed it here, you mentioned that the message should be an
I'm having trouble picturing what this means. GetLocalizedKeys does the below: |
Use an interface for your Error. |
This pull request is now in conflicts. Could you fix it @nick4598? 🙏 |
consumers of itwinjs-core may wish to display a user-friendly message on the frontend involving the ConflictingLocks interface.
Potentially relevant work item: https://github.com/iTwin/itwinjs-backlog/issues/1072#issuecomment-2196966663
About maybe a way we can avoid having to do this for each type of error we may end up throwing.