fix(persist): Using [Array] to persist file

[niheaven]

A new way to handle persist data, esp. with files

With this patch, one can distinguish file and dir in persist property by the following code:

"persist": [
    "dir",
    ["another_dir", "different_persist_name"],
    ["file"],
    ["another_file", "different_persist_name", ""],
    ["third_file", "", "third_file_contents"],
    ["forth_file", "different_persist_name", "forth_file_contents"],
    ["fifth_file", "", "fifth_file", "contents"]
]

persist accepts [Array] as persist object, and array length is checked to determine if it is file or dir, with the following logic:

1. length = 1. Object is file, and file contents is empty string ("", not $null)
2. length = 2. Object is dir, and the second element is the changed persist name
3. length >= 3, Object is file, and the remaining elements is file contents which will be joined with \r\n

if persist object is [String], it is dir without change.

In persisted file contents, every variables that can be used in installer.script can be used, e.g., $persist_dir, $architecture, etc.

It is ready to test and be merged, and I made a modified manifest of nvm.json for testing.

[Ash258]

I would rather see implementation as descibed in #2897 (comment) with small tweak, that content property on file would be array or string

[r15ch13]

Sorry, but I would like to use a more complex object instead of an array. I have posted an example on another persistent topic (but I don't know which one right now)
Something like the following.

"persist": [
      {
          "type": "file",
          "name": "settings.ini",
          "content": "blah",
          "encoding": "UTF-8"
      }
]

Sorry I am on mobile right now

Edit: @Ash258 is one step ahead of me, as always 😁

Sent from my OnePlus5T using FastHub

[Ash258]

@r15ch13 I already mentioned your post

[niheaven]

Yes, file contents could be array, please see fifth file example above. The function treats all lines except the first two as file contents.

The ideal one is perfect, but it means totally rewriting of persist function and is not backward compatible. That's a long time job.

This one doesn't change anything with existing manifests, and can be a temporary solution. It could handle most persisting job with just small change.

[Ash258]

It could be backward compatible. Just keep current functionality and add check if it's just object or string. If string apply current implementation. If it's object, apply new one.

This will provide time for maintaners to reflect new changes and everything will work. Then after few weeks / months old implementation could be removed.

[niheaven]

You're right, we chould even use script to convert persist type from [Array] to [Object].

But why not use this one as backward compatible implementation while the new one is under construction? This COULD do most persisting job well now.

[Ash258]

Because you will make update to reflect these changes and then update manifest to adopt future new changes. It's not worth it's time.

[niheaven]

In my opinion, the procedure should be:

1. Adopt this change
2. Maintainer update manifests to reflect this change, distinguish file and dir persisted and add file contents to persist
3. When the new implementation is arrived, since file and dir is different in this one and file contents is put in a seperate space, an automatic script could be used to convert existing manifests to adopt new persist property type

Maybe the same thing is leting maintainer change their manifests to adopt new persist section and deleting pre_install, but this is just done once, and the following conversion could be done automatically. There isn't additional job to do.

[chawyehsu]

The ideal one is perfect, but it means totally rewriting of persist function and is not backward compatible. That's a long time job.

Your implementation is not backward compatible, it breaks all manifests which have file-type data persistence. The implementation forces maintainers to update all these manifests to reflect your implementation.

diff --git a/shadowsocks.json b/shadowsocks.json
index 72bb2b3..346bb32 100644
--- a/shadowsocks.json
+++ b/shadowsocks.json
@@ -13,10 +13,10 @@
         ]
     ],
     "persist": [
-        "gui-config.json",
-        "statistics-config.json",
-        "pac.txt",
-        "user-rule.txt"
+        ["gui-config.json"],
+        ["statistics-config.json"],
+        ["pac.txt"],
+        ["user-rule.txt"]
     ],

It's not a smooth transition for maintainers and users, which could be a debatable idea I think. Though I don't like this actual implementation, I would do a careful review if you are willing to make it be backward compatible, which means no manifest update needed after adopting your change.

[r15ch13]

Proposed schema change:

        "persist": {
            "anyOf": [
                {
                    "type": "string",
                    "description": "Deprecated, use object representation instead."
                },
                {
                    "items": {
                        "$ref": "#/definitions/stringOrArrayOfStrings"
                    },
                    "minItems": 1,
                    "type": "array",
                    "description": "Deprecated, use object representation instead."
                },
                {
                    "minItems": 1,
                    "type": "array",
                    "items": {
                        "type": "object",
                        "additionalProperties": false,
                        "required": [
                            "name",
                            "type"
                        ],
                        "if": {
                            "properties": {
                                "type": {
                                    "const": "file"
                                }
                            }
                        },
                        "then": {
                            "required": [
                                "encoding",
                                "content"
                            ]
                        },
                        "properties": {
                            "name": {
                                "type": "string"
                            },
                            "target": {
                                "type": "string"
                            },
                            "type": {
                                "enum": [
                                    "file",
                                    "directory",
                                    "folder"
                                ]
                            },
                            "encoding": {
                                "enum": [
                                    "utf-8",
                                    "ascii",
                                    "byte"
                                ]
                            },
                            "content": {
                                "$ref": "#/definitions/stringOrArrayOfStrings"
                            },
                            "method": {
                                "enum": [
                                    "copy",
                                    "merge",
                                    "link",
                                    "update"
                                ]
                            }
                        }
                    }
                }
            ]
        },

[Ash258]

@r15ch13 Will you open PR with these changes to schema? To move discussion there.

[niheaven]

Thanks @h404bi.

In fact it is backward compatible, since now we create file in pre-install before persisting it, and for these existing file, the original procedure isn't broken, it just care about "new" ones. So if this be merged, the maintainer haven't to fix their manifests if they wouldn't do, it's all the same. Only if they like to change, they could delete pre-install and add brackets around persisted file name.

I'll use nvm for example (this is such a typical persist example that it has file persistence with contents, dir persistence and dir renaming persistence). nvm in main bucket is the unchanged one, and nih/nvm is the changed one.

[niheaven]

@r15ch13 Thanks for the schema, and I'll construct a new implementation on it in few days and an automatical conversion script for it.

BTW, I'm not sure what does "merge" mean. For example, if an app persists its plugin dir before, and we install an updated one and require "merge", we should copy new items into existing directory or we should copy items cover it? If we cover it, the existing config for former plugin may become invalid since new one doesn't have their config. So what and why should we do a "merge"?

[r15ch13]

@Ash258 yes
@niheaven yes, merge is for plugins/languages/skins and so on.

[r15ch13]

Alternative #3209

[niheaven]

• Using Remove-Item to remove junction and hardlink
• Apply helper function create_xxx from [WIP] Persistence rework with backward compatibility (persist) #3212
• Seperate persist_data as wrapper function and persist_helper core function (which chould be used for a more common case)

[niheaven]

Close #3210. Add nvm-object.json and nvm-array.json for testing.

[niheaven]

I've done the job, now both [Object] and [Array] are supported, please test and review.

BTW: I'll open a new PR for clear discussion, and attemp to add conversion script.