Essentials: [Bug] SecureStorage Tizen implementation should check for existing alias instead of throwing.

[Mrnikbobjeff]

Description

A few hours ago code was introduced in master which "fixed" our SecureStorage Tizen implementation. It has a nasty try/catch which is needlessly called and may lead to excessive exceptions. We should use the given DataManager.GetAliases and check for alias existance this way instead of deleting on every preference set. See here

[jamesmontemagno]

@DamianMehers Looking at the docs you could call GetAliases() and check to see if it exists, and if so then remove it and then add again.

[DamianMehers]

The exception is only thrown if the key doesn't exist, which only happens the first time the key is written, so I don't understand how it may lead to excessive exceptions.

Assuming that keys are frequently updated and not just written once, getting all keys every time an individual key is written will be more expensive than throwing the exception once on first write.

So the choice is the cost of:

• throwing and catching an exception once, the very first time a specific key is written.
• getting all values every time a key is written, and check.

I don't think that throwing the exception just the first time is such a big deal (although it is ugly and I don't like it), but I'm fine with getting all the keys for every call if you think that is best @jamesmontemagno / @Redth . That will actually be more expensive, assuming people rarely write a specific key just once.

Note that GetAliases itself throws an exception if there are no aliases, so we would be forced to add yet another nasty try/catch.

@Mrnikbobjeff I have the same gut reaction to exceptions being part of a normal flow, but I could not see a less expensive way of doing things if keys are updated more often than they are initially written (the archetypal case being refreshed oauth tokens). Happy to be corrected if there is a flaw in my reasoning.

[Mrnikbobjeff]

We had an application where we used dynamically generated keys to store objects, these objects were removed after every application version update. Many settings were never read again (based on app usage). We would have an exception every time we set a new key, which is superfluous imo. I took a peek at the source code to check exactly what goes on, here are my findings:

1. The return type is always a list, no idea why they decay to ienumerable.
2. The amount of allocations happening are definitely more expensive than the exceptions, expecially because:
3. The Tizen source only acts as an interop wrapper for ckm, and marshalling is expensive because the overloads take a char* and have to be marshalled accordingly
4. The ckm library does not expose the means to check for single key existence, instead it returns an error code if the key does exist when trying toset the key.

The optimal solution is to make a pr to the Tizen SDK to implement this functionality, as they simply should not throw if that error code is thrown, instead expose a way to override existing parameters (deleting and then setting). We could also do this ourselves, which is not as nice as we would have to maintain that code. For now though I have to agree, the exception is probably less expensive.

[DamianMehers]

For now though I have to agree, the exception is probably less expensive.

@Mrnikbobjeff Should we close this bug?

[jamesmontemagno]

Will close for now.

[Mrnikbobjeff]

I added a PR for TizenFX exposing the required functionality. Will ping this once it is merged and published in the next Tizen API update if it should come this far