Calling AddOrUpdateValue with a string throws "Value of type String is not supported"

[opcodewriter]

Calling AddOrUpdateValue with string type throws exception Value of type String is not supported. Here's how this is possible:

object strValue = "a";

CrossSettings.Current.AddOrUpdateValue(key, strValue);

This is because the implementation is using Type.GetTypeCode to get the primitive type, but this returns System.TypeCode.Object, and not System.TypeCode.String as you could expect.

https://github.com/jamesmontemagno/SettingsPlugin/blob/master/src/Plugin.Settings.Android/Settings.cs#L207

Same issue is on iOS, it's very similar code.

As a solution, maybe use Convert.GetTypeCode which correctly returns TypeCode.String?

[jamesmontemagno]

You can't pass it an object like that, you have to make it:

string strValue = "a";

[opcodewriter]

Thanks but I know that using a string fixes it, that's not the point.

I'm confused, why did you close the issue. It is an actual issue, the current implementation does not handle the values correctly.

[jamesmontemagno]

I have asked twitter about getting the type: https://twitter.com/JamesMontemagno/status/861439843334725632

[jamesmontemagno]

Since the method is generic you are not saying it is a string because you are passing it an object. object types are not supported: https://github.com/jamesmontemagno/SettingsPlugin#data-types-supported

and they will never be due to limitations on specific platforms. If it is a string make it a string or do:

CrossSettings.Current.AddOrUpdateValue<string>(key, (string)strValue);

[ncarandini]

Then surely the error message is wrong :-) It should say "Value of type object is not supported".

[jamesmontemagno]

Yes, i should pass typecode :) which i already have, will fix that.

[opcodewriter]

I think the AddOrUpdateValue method in the current form is leaking the details of its implementation in too many ways. If it's not actually supporting an object but only primitive values, then I think it's best to have overloaded methods for each type it supports. Or at least change the method name to AddOrUpdatePrimitiveValue \ SimpleValue.

But going back to this specific issue: why can't it be changed to call Convert.GetTypeCode? It would fix the issue, I don't see where's the problem with GetTypeCode.

[ncarandini]

The point here is that the object passed is a string, not an object so it can be saved to settings. The problem is not Type.GetTypeCode() Vs. Convert.GetTypeCode() but on what you pass as parameter of Type.GetTypeCode(), losing the "I'm an object but in fact I'm a string" information. I've made a quick test with Linqpad, using the demo code of the Type.GetTypeCode() Microsoft reference page passing an object that is a string:

void Main()
{
    object myStr = "Hello World!";
    WriteObjectInfo(myStr);
}

static void WriteObjectInfo(object testObject)
{
    TypeCode typeCode = Type.GetTypeCode(testObject.GetType());
    Console.WriteLine("{0}: {1}", typeCode.ToString(), testObject);
}

and the Type.GetTypeCode() do return: String: Hello World!

But when using a generic method and passing the typeOf to the Type.GetTypeCode() method as you do in your code:

Type typeOf = typeof(T); 
...
var typeCode = Type.GetTypeCode(typeOf);

the "object as a string" information is lost, as you can test with this modified code:

void Main()
{
    object myStr = "Hello World!";
    GenericWriteObjectInfo(myStr);
}

static void GenericWriteObjectInfo<T>(T obj)
{
    Type typeOf = typeof(T);
    TypeCode typeCode = Type.GetTypeCode(typeOf);

    Console.WriteLine("{0}: {1}", typeCode.ToString(), obj);
}

that return: Object: Hello World!.

So I suggest to skip the Type.GetTypeCode() Vs. Convert.GetTypeCode() battle, reopen this issue and use whatever method is best suited to handle the "object as a string".

At the end of the day, the object passed is a string (as the error message correctly reports) so I do think the method code can be enhanced to support it.

As an example of still using Type.GetTypeCode() I suggest to change your code to:

var typeCode = Type.GetTypeCode( value.GetType() );

I've just tested it and it works, returning TypeCode.String.

[opcodewriter]

I got a bit confused. I think the actual issue is with Type typeOf = typeof(T); here:

https://github.com/jamesmontemagno/SettingsPlugin/blob/master/src/Plugin.Settings.Android/Settings.cs#L202

It should instead be Type typeOf = value.GetType();

When using GetType(), the value of typeOf is a System.String, hence the implementation will correctly see it as a string and fixes the issue.

typeof is compile-time bound while GetType() is evaluated at runtime.

[Tornhoof]

Keep in mind that .GetType() does not work with null, Convert.GetType() solves that problem by returning TypeCode.Empty. But since TypeCode.Empty is not supported anyway in your private AddOrUpdateValue method you can simply replace it with a ArgumentNullCheck. You probably might want to add a non-generic AddOrUpdateValue method and use value.GetType() and an ArgumentNullCheck.

[ncarandini]

@opcodewriter That's what I've suggested :-) I've preferred to use it directly inside the var typeCode = Type.GetTypeCode( value.GetType() ); line of code to not interfere with the other previous code. Moreover, it was only a suggestion to show the point I was arguing to reopen the issue. The right thing to do about real code change is to create and submit a PR.

[opcodewriter]

@Tornhoof good point about supporting null value. About non-generic version: Note you still need to know the actual type and call the right platform method. For example, on Android, it must still call sharedPreferencesEditor.PutString which with the null value it removes the preference. So the method must work with actual type, a non-generic AddOrUpdateValue won't work I think. If we want non-generic version, it would be better to have something like AddOrUpdateValueAsString(string key, object value) which will support any value (including complex types), it would serialize it to a string and store it in the platform preferences\settings as a string.

Again, to me I still think the current design of the method is a problem. It looks like you can store any value with it, when in fact it only supports primitive values AND only if you actually pass them by their actual type, not like I'm doing. Too many conditions. Yes, I know these can be documented and throw exceptions but this is code smell to me, not sure.

[Tornhoof]

@opcodewriter I would simply do something like this as the non-generic version:

public bool AddOrUpdateValue(string key, object value)
{
  if(value == null)
  {
    throw new ArgumentNullException(nameof(value));
  }
   return AddOrUpdateValue(key, value, Type.GetTypeCode(value.GetType()));
}

The already existing private AddOrUpdateValue is unchanged and handles all the cases, including the typecode specific ones. I don't have any opinion about complex types, as they are not part of the current code.

[opcodewriter]

@Tornhoof so the generic version of works with null but not the non-generic version? :(

[opcodewriter]

If the non-generic version doesn't work with null, how will you remove that value?

[opcodewriter]

Oh, there's a Remove method... But in this case why does the current AddOrUpdateValue work with null? To me it appears it does, there's no check for null.

[opcodewriter]

I think your solution works, although it's a bit confusing when you will look at the two methods. One is a generic one and the other is a specialization. It's hard to explain the difference and the need for the two without describing a bit the details. But it will do the job I think and fix the issue.

[Tornhoof]

The current generic one works with null, the typecode of null is TypeCode.Empty but TypeCode.Emtpy throws an ArgumentException in the private method. Either you deprecate the generic one, as it is not really necessary or simply call the nongeneric one.

[opcodewriter]

Sorry, I should have been more clear. I was referring to this:

string s = null;
AddOrUpdateValue("myKey", s);

I think this currently works, no? And it removes the value, just like when you call Remove. Haven't tested it but it looks so.

[Tornhoof]

Yes, I didn't see any code which actually parses the content of the object.

[jamesmontemagno]

it mostly comes down to 2 things..

1.) I don't want to support "object as a string" 2.) Passing in null is not supported

[jamesmontemagno]

I have added some new checks. It is generic as it is easier to maintain, however I can easily remove this and make it so you are only able to pass in the specific types that I allow. No one has complained about this in about 4 years... but I will do what is right for the library, without breaking compat.

[opcodewriter]

Thanks, it's OK I guess. I refactored some things on my side.