moin icon indicating copy to clipboard operation
moin copied to clipboard

Add check to cli commands to prevent accidentally destroying items or indexes

Open UlrichB22 opened this issue 1 year ago • 6 comments

CLI commands should have checks to prevent destroying items, indexes or an existing wiki. For example, if moin index-build is executed twice, each item will be included in the index twice. Maybe there are others.

There is a note in the docs about index-build that needs to be updated if check has been added: https://moin-20.readthedocs.io/en/latest/admin/index.html#moin-index-build

IMO there is a note missing in this doc to clear the index if it is corrupt but still in place: https://moin-20.readthedocs.io/en/latest/admin/index.html#if-your-wiki-has-data-and-is-shut-down

UlrichB22 avatar Apr 24 '23 15:04 UlrichB22

I found that ./m = quickinstall.py does those checks using the function https://github.com/moinwiki/moin/blob/94c665fdb72409f12a64dcf94c191e8e6acf4ac9/quickinstall.py#L185-L187

Some commands in quickinstall.py are wrapping moin cli calls with some additional checks and logging. IMO we should reduce redundancy and move all the useful code from these wrapper functions into the moin cli calls. The original ./m commands should get a deprecated warning and can be removed in a later version.

The command ./m should only be used for development purposes like installation of dev-tools, running pytest and pip updates. The moin cli should have all functions needed by a wiki administrator.

I am aware that there was an old discussion and different opinions about the use of ./m and moin cli. We should avoid redundant code and give users a clear orientation which tools to use in which situation.

UlrichB22 avatar May 04 '23 20:05 UlrichB22

Agree, but no need to add deprecated messages to ./m commands - just remove them.

Not sure if you already fixed it but one issue with the moin commands was doing the commands out of order yielded confusing tracebacks (trying to run before creating a wiki instance, etc.).

RogerHaase avatar May 04 '23 21:05 RogerHaase

The above PR adds checks to CLI commands which will raise an error if the index is needed but missing. Even 'moin run' will give an error message instead of a traceback, if the instance is created but the index is missing.

For the initial issue of index-build running twice on an existing wiki I need some help. The wiki_index_exists() function does not distinguish between an initially created empty index and a filled index.

I will remove the index (and run?) command from quickinstall.py in one of the next PRs.

UlrichB22 avatar Mar 06 '24 18:03 UlrichB22

Does this help in determining whether an index exists and has content?

https://whoosh.readthedocs.io/en/latest/indexing.html#clearing-the-index

RogerHaase avatar Mar 07 '24 21:03 RogerHaase

Does this help in determining whether an index exists and has content?

https://whoosh.readthedocs.io/en/latest/indexing.html#clearing-the-index

I tried the whoosh index.exists_in() function, but the result is the same as our wiki_index_exists() function. Rather than go into more detail about the safety of moin index-build, I prefer to reduce the need for it.

IMO moin index-build is only necessary and useful in case of an empty index after running moin load (restore) or moin load-sample.

  • We have already discussed removing load-sample as it is almost covered by help-de and help-common, see #1155.
  • When we run load to restore from a backup, we can include the index build at the end as in import19.
  • moin index-update can be used as a replacement for index-build anyway.

Did I miss something?

UlrichB22 avatar Mar 12 '24 15:03 UlrichB22

Sounds OK to me

RogerHaase avatar Mar 12 '24 20:03 RogerHaase