[stable32] fix(ContactsManager): Fix conflict with value type from database

[solracsf]

Summary

⚠️ UNIFIED SEARCH STOPPED WORKING

1. Try a search (search never returns results)
2. Observe search console (and Nextcloud error log)

Regression from #55657
Cc @artonge

Fixes:

# occ config:app:get dav system_addressbook_exposed
no

"Exception": "OCP\\Exceptions\\AppConfigTypeConflictException",
    "Message": "conflict with value type from database",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/nextcloud/lib/private/AppConfig.php",
        "line": 413,
        "function": "getTypedValue",
        "class": "OC\\AppConfig",
        "type": "->",
        "args": [
          "dav",
          "system_addressbook_exposed",
          "true",
          false,
          32
        ]
      },
      {
        "file": "/var/www/nextcloud/apps/dav/lib/CardDAV/ContactsManager.php",
        "line": 48,
        "function": "getValueBool",
        "class": "OC\\AppConfig",
        "type": "->",
        "args": [
          "dav",
          "system_addressbook_exposed",
          true
        ]
      },

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

[solracsf]

If this get merged, should be backported until stable28 like original commit d14c96b

PS: #55657 should also be reviewed (for master, not same codebase)

[susnux]

But this wrongly writes a string? This sounds more like there is a place where it explicitly is written as string (legacy app config writes as mixed which should be ok?), no?

[come-nc]

This looks super wrong.
There is a lexicon for this application setting typing it to bool.

[come-nc]

Ah, 32 does not have the lexicon, it was not backported. Maybe it’s related to the issue.

@solracsf Do you know how the value got typed to string? I check and occ config:app:set types to mixed by default, not string. So I do not see how this value could end up being typed as string in DB.

[artonge]

@ArtificialOwl shouldn't the config API handle the type conversion? Am I missing something?

EDIT: I think the missing lexicon is indeed necessary to enforce a new type on an already existing config.

[artonge]

Needed indeed 👍

[artonge]

/backport to stable31

[artonge]

/backport to stable30

[artonge]

/backport to stable29

[artonge]

/backport to stable28

[solracsf]

@come-nc

Ah, 32 does not have the lexicon, it was not backported. Maybe it’s related to the issue.

@solracsf Do you know how the value got typed to string? I check and occ config:app:set types to mixed by default, not string. So I do not see how this value could end up being typed as string in DB.

I can't really tell, this is set since several major versions already, and it only broke on upgrade to 31 😿

[come-nc]

@solracsf But that’s only on one instance right?
The getValueString will break on instances where the variable is typed as bool.
And there is at least one other getValueBool on the same setting.

So, this is not mergeable in my opinion, it should either migrate the setting to bool, or get it as untyped using the legacy API if needed. And it should fix all calls not only this one.

[solracsf]

No problem, I can easily type it to bool on my side of course. I'll let you check with @artonge if this is needed or not. 👍🏼

[ArtificialOwl]

would be nice to understand why the value type is set as string in the database. If this is a local issue only on a single instance:

./occ config:app:set --type boolean dav system_addressbook_exposed

if this needs to be applied to everyone, one of the solution would be a repair step using IAppConfig::updateType()

We could also add an option to Lexicon/Entry to enforce the updateType if conflict exists:
#56277

[SebastianKrupinski]

@ArtificialOwl the original value was a string "yes" this probably did not get coverted properly

[ArtificialOwl]

The value will always be stored in a string in the database, could it be 'yes', 'true', '1', 'on', but will be returned as boolean by getValueBool()
The exception here is because the value is marked as 'string' (value type) in the database while the method called expect the value to be stored as 'boolean'.

[ArtificialOwl]

(should not be merged as it is)

[ArtificialOwl]

No problem, I can easily type it to bool on my side of course. I'll let you check with @artonge if this is needed or not. 👍🏼

Hello @solracsf, does it means you changed the expected type of the config value yourself before ? We are trying to understand if the value was set to string (from mixed) by code or by human interaction

[solracsf]

I can't tell, but there is high chance it has been set trough occ to 'no' value.

[ArtificialOwl]

By default, value are stored as mixed in the database, unless using --type string when running ./occ config:app:set or value type is already defined in database

[solracsf]

Yeah sure, but since this has been set years ago, I can't know for sure how it has been set, sorry.

I would say that, since there are no other reports since the release, maybe this isn't a big deal and PR can be closed. 👍🏼