unsignedapps / Vexil

Vexil (named for Vexillology) is a Swift package for managing feature flags (also called feature toggles) in a flexible, multi-provider way.
https://vexil.unsignedapps.com
MIT License
115 stars 12 forks source link

Crash when editing optional enum flag in Vexilographer #113

Open orj opened 1 year ago

orj commented 1 year ago

Optional enum flags seem to Crash Vexilographer when going into edit mode.

Vexil version: 2.2.1 Swift version: swift-driver version: 1.75.2 Apple Swift version 5.8.1 (swiftlang-5.8.0.124.5 clang-1403.0.22.11.100) Environment: Version 14.3.1 (14E300c)

✅ Checklist

- [ ] If possible, I've reproduced the issue using the main branch of this package

🔢 Steps to Reproduce

Replace this paragraph with an explanation of how to reproduce the incorrect behavior. This could include a code listing for a reduced version of your command, or a link to the code that is exhibiting the issue.

🎯 Expected behavior

No crash. Things work as expected.

🕵️‍♀️ Actual behavior

I have defined a flag like this:

@Flag(name: "Application User Mode", default: nil, description: "The application mode for users")
    public var applicationUserMode: ApplicationUserMode?

The ApplicationUserMode is defined as:

public enum ApplicationUserMode: String {
    /// Users are enterprise employees
    case enterpriseEmployees

    /// Users are Apple App Store subscribers
    case individualSubscribers
}

extension ApplicationUserMode: FlagValue, FlagDisplayValue {
    public var flagDisplayValue: String {
        switch self {
        case .enterpriseEmployees:
            return "Enterprise Employees"
        case .individualSubscribers:
            return "Individual Subscribers"
        }
    }
}

When trying to edit this in Vexilographer it crashes with the following stack trace.

[User Defaults] Attempt to set a non-property-list object <null> as an NSUserDefaults/CFPreferences value for key application-user-mode
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: 'Attempt to insert non-property list object null for key application-user-mode'
*** First throw call stack:
(
    0   CoreFoundation                      0x0000000180437330 __exceptionPreprocess + 172
    1   libobjc.A.dylib                     0x0000000180051274 objc_exception_throw + 56
    2   CoreFoundation                      0x00000001804bd1c8 -[__NSCFString characterAtIndex:].cold.1 + 0
    3   CoreFoundation                      0x00000001803f7158 _CFPrefsValidateValueForKey + 216
    4   CoreFoundation                      0x00000001803f77d4 createDeepCopyOfValueForKey + 160
    5   CoreFoundation                      0x00000001803f74f0 -[CFPrefsSource setValues:forKeys:count:copyValues:removeValuesForKeys:count:from:] + 320
    6   CoreFoundation                      0x00000001803f78b8 -[CFPrefsSource setValue:forKey:from:] + 68
    7   CoreFoundation                      0x00000001804a0338 __76-[_CFXPreferences setValue:forKey:appIdentifier:container:configurationURL:]_block_invoke + 56
    8   CoreFoundation                      0x000000018045fbe4 __108-[_CFXPreferences(SearchListAdditions) withSearchListForIdentifier:container:cloudConfigurationURL:perform:]_block_invoke + 344
    9   CoreFoundation                      0x000000018045f384 normalizeQuintuplet + 348
    10  CoreFoundation                      0x000000018045fa64 -[_CFXPreferences withSearchListForIdentifier:container:cloudConfigurationURL:perform:] + 124
    11  CoreFoundation                      0x00000001804a02d8 -[_CFXPreferences setValue:forKey:appIdentifier:container:configurationURL:] + 100
    12  CoreFoundation                      0x00000001804a4578 _CFPreferencesSetAppValueWithContainerAndConfiguration + 116
    13  Foundation                          0x0000000180bd06e4 -[NSUserDefaults(NSUserDefaults) setObject:forKey:] + 64
    14  ThePeopleSpotSettings               0x000000010499e97c $sSo14NSUserDefaultsC5VexilE12setFlagValue_3keyyxSg_SStKAC0eF0RzlF + 492
    15  ThePeopleSpotSettings               0x000000010499f24c $sSo14NSUserDefaultsC5Vexil15FlagValueSourceA2cDP03setdE0_3keyyqd__Sg_SStKAC0dE0Rd__lFTW + 24
    16  ThePeopleSpotSettings               0x000000010498d814 $s5Vexil4FlagV4save2toyAA0B11ValueSource_p_tKF + 268
    17  ThePeopleSpotSettings               0x000000010498d900 $s5Vexil4FlagVyxGAA03AnyB0A2aEP4save2toyAA0B11ValueSource_p_tKFTW + 40
    18  ThePeopleSpotSettings               0x000000010498b4f4 $s5Vexil8FlagPoleC4save8snapshot2toyAA8SnapshotCyxG_AA0B11ValueSource_ptKFyAA03AnyB0_pKXEfU_ + 92
    19  ThePeopleSpotSettings               0x000000010498b54c $s5Vexil8FlagPoleC4save8snapshot2toyAA8SnapshotCyxG_AA0B11ValueSource_ptKFyAA03AnyB0_pKXEfU_TA + 24
    20  libswiftCore.dylib                  0x000000018bd57490 $sSTsE7forEachyyy7ElementQzKXEKF + 440
    21  ThePeopleSpotSettings               0x000000010498b430 $s5Vexil8FlagPoleC4save8snapshot2toyAA8SnapshotCyxG_AA0B11ValueSource_ptKF + 180
    22  ThePeopleSpotSettings               0x0000000104e20068 $s14Vexillographer16FlagValueManagerC03setbC0_3keyyqd__Sg_SStK5Vexil0bC0Rd__lF + 296
    23  ThePeopleSpotSettings               0x0000000104e206a0 $s14Vexillographer16FlagValueManagerC08setBoxedC0_4type3keyy0fC4TypeQyd__Sg_qd__mSStK5Vexil0bC0Rd__lF + 404
    24  ThePeopleSpotSettings               0x0000000104dea5dc $s7SwiftUI7BindingV14VexillographerE3key7manager12defaultValue11transformerACyxGSS_AD04FlagH7ManagerCyqd_0_Gqd_1_qd__mtc07EditingH0Qyd__RszAD05BoxedjH11TransformerRd__5Vexil0J9ContainerRd_0_AP0jH0Rd_1_0mH4TypeQyd_1_08OriginalH0Rtd__r1_lufcyxcfU0_ + 412
    25  ThePeopleSpotSettings               0x0000000104dea95c $s7SwiftUI7BindingV14VexillographerE3key7manager12defaultValue11transformerACyxGSS_AD04FlagH7ManagerCyqd_0_Gqd_1_qd__mtc07EditingH0Qyd__RszAD05BoxedjH11TransformerRd__5Vexil0J9ContainerRd_0_AP0jH0Rd_1_0mH4TypeQyd_1_08OriginalH0Rtd__r1_lufcyxcfU0_TA + 76
orj commented 1 year ago

I have noticed in extension UserDefaults: FlagValueSource method setFlagValue<Value>() that value is a ApplicationUserMode?? (note the double optional). The outer optional is non-nil as it contains the inner nil. So the initial guard let value = value passes but when attempting to insert the value.boxedFlagValue it resolves to nil.

orj commented 1 year ago

Perhaps I'm holding it wrong?

orj commented 1 year ago

I note that if I make the enum CaseIterable it doesn't crash. But the editor is a bit "meh" and doesn't work quite right.

https://github.com/unsignedapps/Vexil/assets/34415/dfcf743e-dde1-462c-ad86-053f9ad53bd6

huwr commented 1 year ago

I feel like the last problem could perhaps be addressed or improved by https://github.com/unsignedapps/Vexil/pull/112

patANZx commented 1 year ago

Pretty sure the crash occurs here: https://github.com/unsignedapps/Vexil/blob/6b8a039d4fffe105b717b08df3e8eb6b588e0ea3/Sources/Vexil/Sources/UserDefaults%2BFlagValueSource.swift#L43-L51

One way to stop the crash would be to use nil when .boxedFlagValue.object == NSNull():

if value.boxedFlagValue.object == NSNull() {
  set(nil, forKey: key)
} else {
  set(value.boxedFlagValue.object, forKey: key)
}

This doesn't quite work for setting optional values though. It simply removes the flag rather than setting it to nil. Using Data() instead of nil could be an option. I'm not 100% on how UserDefaults differs across platforms and not too familiar with Vexil so opted to leave it out of #112.

A small change to OptionalCaseIterableFlagControl would also be needed.

@orj Here's a branch with the above changes. Any chance you could give it a test drive and make sure it doesn't break anything else? I was only testing with an optional case iterable control too. https://github.com/unsignedapps/Vexil/compare/main...patANZx:Vexil:fix/vexillographer-optional-flags

patANZx commented 1 year ago

I have noticed in extension UserDefaults: FlagValueSource method setFlagValue() that value is a ApplicationUserMode?? (note the double optional). The outer optional is non-nil as it contains the inner nil. So the initial guard let value = value passes but when attempting to insert the value.boxedFlagValue it resolves to nil.

@orj Oh I missed this message. Didn't realise you had already figured it out :P Not sure if the above fix is suitable but it's been working okay with the OptionalCaseIterableFlagControl for me.

huwr commented 1 year ago

The double-optional problem is something I also encountered but never bothered to investigate: https://github.com/unsignedapps/Vexil/pull/109 so if deemed suitable your solution would fix a few

patANZx commented 1 year ago

@huwr I'm guessing some more work would need to be done with the text editor to get it working fully. I haven't used it much but I remember having a hard time with optional numbers.