repairing parentid for destroy rev, fixes #1448 #1455

bylsmad · 2023-05-21T13:58:36Z

repairing parentid for destroy rev, fixes #1448

also updating maint-validate-metadata, final fix for #1402

updating index when fixing items
not replacing revision numbers for gaps in sequence

also updating maint-validate-metadata, final fix for moinwiki#1402 * updating index when fixing items * not replacing revision numbers for gaps in sequence

bylsmad · 2023-05-21T13:59:45Z

src/moin/storage/middleware/_tests/test_protecting.py

@@ -113,6 +115,14 @@ def test_destroy_revision(self):
        with pytest.raises(AccessDenied):
            item.destroy_revision(revid_protected)

+    def test_destroy_middle_revision(self):
+        item, item_name, revid0, revid1, revid2 = self._store_three_revs('joe:read,write,destroy')


destroying a middle revision now requires write permission as well as destroy in order to allow for update of the parentid

RogerHaase · 2023-05-21T21:05:05Z

If I create an item with 3 revisions and delete revision 1, then create a new revision; the revision number of the new revision is 3; should be 4. Item History shows revisions 2,3,3.

moin maint-validate-metadata shows no errors.

bylsmad · 2023-05-22T13:07:01Z

I will reinstate some of the revision_number fixing in maint-validate-metadata, sounds like the rule is the revision_numbers are allowed to have gaps but not repeats?

RogerHaase · 2023-05-22T14:17:30Z

right, revision numbers may have gaps, not repeats

also correcting index update to search in ALL_REVS existing_item only searches LATEST_REVS

bylsmad · 2023-05-23T15:26:03Z

latest commit adds validation for repeated revision_number, also fixed issue with the way I was querying the index, previous version only got hits from LATEST_REVS this commit finds revs in ALL_REVS

RogerHaase · 2023-05-23T19:24:29Z

Oops updated issue, meant to update here:

Testing with an item with 4 revisions...

If rev-number 1 (or lowest rev-number) is destroyed, rev-number 2 is updated, and becomes the current revision with updated timestamp. Item history shows rev-numbers 2, 4, 3.

Testing with another item with 4 revisions...

If rev-number 2 is destroyed, then the timestamp for rev-number 3 is updated and becomes current. Item history shows rev-numbers 3, 4, 1.

I think the metadata timestamp (or MTIME) should refer to the update time of the revision data. Updating the metadata should not change the metadata MTIME. Is there a way to update the parentid without updating the MTIME?

RogerHaase · 2023-05-28T16:57:24Z

Hang in there, I know this is a hard one...

2023-05-28 09:41:16,639 ERROR werkzeug:224 Error on request:
Traceback (most recent call last):
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\werkzeug\serving.py", line 333, in run_wsgi
    execute(self.server.app)
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\werkzeug\serving.py", line 320, in execute
    application_iter = app(environ, start_response)
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 2091, in __call__
    return self.wsgi_app(environ, start_response)
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 2076, in wsgi_app
    response = self.handle_exception(e)
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 2073, in wsgi_app
    response = self.full_dispatch_request()
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 1518, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 1516, in full_dispatch_request
    rv = self.dispatch_request()
  File "C:\git-bylsmad\moin-venv-python\lib\site-packages\flask\app.py", line 1502, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**req.view_args)
  File "c:\git-bylsmad\moin\src\moin\apps\frontend\views.py", line 1197, in destroy_item
    item.destroy(comment=comment, destroy_item=destroy_item, subitem_names=subitem_names)
  File "c:\git-bylsmad\moin\src\moin\items\__init__.py", line 771, in destroy
    self.rev.item.destroy_revision(self.rev.revid)
  File "c:\git-bylsmad\moin\src\moin\storage\middleware\protecting.py", line 432, in destroy_revision
    self.item.destroy_revision(revid)
  File "c:\git-bylsmad\moin\src\moin\storage\middleware\indexing.py", line 1291, in destroy_revision
    self.store_revision(child_meta, child_rev.data, overwrite=True, trusted=True)
  File "c:\git-bylsmad\moin\src\moin\storage\middleware\indexing.py", line 1225, in store_revision
    raise ValueError(_('Error: metadata validation failed, invalid field value(s) = {0}'.format(
ValueError: Error: metadata validation failed, invalid field value(s) =

bylsmad · 2023-05-28T17:34:51Z

I've got the metadata validation failed fixed in my fork by allowing userid to be Unset

Now struggling with index files left open by whoosh, but I have the failure reproducible on Linux now so hoping to have a clean test run soon

ERROR src/moin/_tests/test_user.py::TestUser::test_recovery_token - PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\a\\moin\\moin\\src\\moin\\_tests\\wiki\\index\\all_revs_8zhbn7j04fuf7rlb.seg'

latest test fail: https://github.com/bylsmad/moin/actions/runs/5102359184/jobs/9171871358

previous code when destroying a middle revision the update to PARENTID was making the next revision into the latest applying same fix ValidateMetadata

bylsmad · 2023-05-29T18:41:54Z

all tests passing ,ready for review

bylsmad · 2023-05-29T18:48:15Z

src/moin/storage/middleware/indexing.py

@@ -1240,7 +1254,8 @@ def store_revision(self, meta, data, overwrite=False,
        data.seek(0)  # rewind file
        backend_name, revid = backend.store(meta, data)
        meta[REVID] = revid
-        self.indexer.index_revision(meta, content, backend_name)
+        self.indexer.index_revision(meta, content, backend_name, force_latest=not overwrite)
+        gc.collect()  # triggers close of index files from is_latest search


let me know what you think of adding gc.collect() here, it solves the test failure seen in https://github.com/bylsmad/moin/actions/runs/5102359184/jobs/9171871358

it is also possible to solve the test failure by putting the gc.collect just before destroy_app() in src.moin.conftest
but I was worried this would mask potential issues where unclosed files could lead to "too many open files" exception

I will defer to you as where to put gc.collect().

The code seems to work well, but tests on Windows 10 came up with one error:

================================== FAILURES =================================== ________________________ TestSiteCrawl.test_home_page _________________________ self = <moin.cli._tests.test_scrapy_crawl.TestSiteCrawl object at 0x000001F70A1768F0> crawl_results = [] def test_home_page(self, crawl_results): > assert len(crawl_results) > 0 E assert 0 > 0 E + where 0 = len([]) C:\git-david\moin\src\moin\cli\_tests\test_scrapy_crawl.py:54: AssertionError ----------------------------- Captured log setup ------------------------------

Could you attach these files from the failed run:

_test_artifacts/crawl.log
_test_artifacts/server-crawl.log
_test_artifacts/crawl.csv

RogerHaase · 2023-05-29T21:26:46Z

crawl.log
server-crawl.log
crawl.csv

bylsmad · 2023-05-29T23:09:39Z

try latest commit, previous one had a typo in the logging setup for crawl.log and I've added logging to make timeouts easier to see, hopefully you will get same error and we can see what happened

RogerHaase · 2023-05-30T16:03:39Z

================================== FAILURES ===================================
________________________ TestSiteCrawl.test_home_page _________________________

self = <moin.cli._tests.test_scrapy_crawl.TestSiteCrawl object at 0x000001FE42C423E0>
crawl_results = []

    def test_home_page(self, crawl_results):
>       assert len(crawl_results) > 0
E       assert 0 > 0
E        +  where 0 = len([])

C:\git-david\moin\src\moin\cli\_tests\test_scrapy_crawl.py:54: AssertionError
----------------------------- Captured log setup ------------------------------
INFO     moin.cli._tests:__init__.py:33 running ['moin', 'index-build']
INFO     moin.cli._tests:__init__.py:33 running ['moin', 'run', '-p', '9080']
INFO     moin.cli._tests.conftest:conftest.py:162 starting crawl
INFO     moin.cli._tests:__init__.py:33 running ['scrapy', 'crawl', '-a', 'url=http://127.0.0.1:9080/help-en', 'ref_checker']
============================== warnings summary ===============================

crawl.log
server-crawl.log
crawl.csv

bylsmad · 2023-05-31T13:15:14Z

still no clues to the test failure in the logs. something is stopping the spider before it completes.

In both of the failures, the spider stopped at right around the 2 minute point. Do you have any custom pytest.ini with a timeout setting? Could you attach full output of pytest (i.e. m-tox.txt)?

RogerHaase · 2023-05-31T16:12:08Z

No pytest.ini, but have a tox.ini under version control. Github does not support .ini, so here is txt version.

tox.ini.txt
m-tox.txt

Here are fresh copies of *crawl*

crawl.log
server-crawl.log
crawl.csv

bylsmad · 2023-05-31T23:08:03Z

I'm still stumped...what could be different between your workstation setup and the Windows box which runs the test in the github action?

does the test fail when invoked directly using command line?

pytest cli/_tests/test_scrapy_crawl.py

bylsmad · 2023-06-01T11:59:01Z

@RogerHaase another troubleshooting method, manually run the commands from test_scrapy

NOTE: my commands are from Linux terminal, you will need to adjust for Windows

In one window:

# from the root of your git clone, 
. ./activate
cd _test_artifacts/
rm server-crawl.log crawl.csv crawl.log
mkdir cli
cd cli
moin create-instance
moin index-create
moin welcome
moin load-help -n common
moin load-help -n en
moin run -p 9080

NOTE: the test currently does an index-build after the load-help, just realized this is not needed and have filed a new ticket to eliminate the redundant index-build commands #1458

validate that your server is running by going to http://localhost:9080/Home in a browser

In a second window:

. ./activate
cd src/moin/cli/_tests/scrapy/
scrapy crawl -a url=http://127.0.0.1:9080/help-en ref_checker

once the scrapy command finishes, check for existence of _test_artifacts/crawl.csv there should be something like 421 lines in the file

If this does not succeed, please attach the console output of the scrappy command
If it does succeed maybe interesting to see whether it fails if you do the redundant index-build before the emoin run commmand

RogerHaase · 2023-06-01T20:39:23Z

I am confused and must rerun your tests. I was about to claim user error because I created a fresh clone and ran m tests without doing m extras (which installs scrappy in venv).

After correcting the problem by doing m extras and rerunning tests I get the same error? The same error whether scrappy is installed or not?

I ran your suggested tests above and they seem to have worked successfully.

I am distracted by other tasks so I may need a day or two to rerun everything.

fixing couple more bad file paths

bylsmad · 2023-06-02T04:59:00Z

pull down the latest from this branch before you resume testing, I've added more exception traps which should hopefully leave us some clues in crawl.log

one thing I encountered as I was testing error conditions is I had scrapy installed in both my venv and also my user dir and also in the system dir, I got strange errors when scrapy was coming from my user dir or the system dir

I did a closer look at your logs and it appears that the entire crawl is getting completed, but something is going wrong with the output of crawl.csv in spider_closed hopefully the increased logging will show what is happening

error was caused by AsyncWriter which is not used when overwrite is True

bylsmad · 2023-06-02T18:57:16Z

sorry for all the commits, but the final test failure was a race condition which only happened on the github host

I can squash them down once we have scrapy test passing on your workstation

RogerHaase · 2023-06-02T19:56:17Z

Here are the latest logs:

m-tox.txt
crawl.csv
crawl.log
server-crawl.log

RogerHaase · 2023-06-02T20:22:31Z

Here is server log from manual run:

manual.txt

Need anything else?

bylsmad · 2023-06-02T21:57:37Z

ok should be fixed now

the difference is you are running the tests via tox via ./m tests when I run via tox I see the crawl.csv is written to one place and read from another (logging from the latest commit):

INFO     moin.cli._tests.conftest:conftest.py:199 reading /home/bylsmad/PycharmProjects/moin/_test_artifacts/crawl.csv
INFO: writing 420 to /home/bylsmad/PycharmProjects/moin/.tox/py39/lib/python3.9/_test_artifacts/crawl.csv

btw I'm heading out on a bicycle tour for a week, this should be ready for merge now assuming the fix for ./m tests in your environment is working. see you on the other side

RogerHaase · 2023-06-03T14:25:02Z

Horay all tests pass> Have a fun trip.

repairing parentid for destroy rev, fixes moinwiki#1448

efcf06a

also updating maint-validate-metadata, final fix for moinwiki#1402 * updating index when fixing items * not replacing revision numbers for gaps in sequence

bylsmad commented May 21, 2023

View reviewed changes

fixing repeated rev_number, fixes moinwiki#1448

f7ecf94

also correcting index update to search in ALL_REVS existing_item only searches LATEST_REVS

destroy_rev no longer updates MTIME, fixes moinwiki#1448

ee698f2

previous code when destroying a middle revision the update to PARENTID was making the next revision into the latest applying same fix ValidateMetadata

bylsmad force-pushed the 1448-destroying-a-revision-breaks-parentid-chain branch from f2d368c to ee698f2 Compare May 29, 2023 18:16

bylsmad commented May 29, 2023

View reviewed changes

fixing logging for scrapy crawl.log

e62b3b3

improving scrapy error reporting

6888aaa

fixing couple more bad file paths

bylsmad added 2 commits June 2, 2023 05:45

fix for logic for _async in store_rev, moinwiki#1448

f3e83fb

fixing race contidion in test, moinwiki#1448

6842b33

error was caused by AsyncWriter which is not used when overwrite is True

fix for scrapy test running under tox

7fe23f9

bylsmad force-pushed the 1448-destroying-a-revision-breaks-parentid-chain branch from c4fa6d0 to 7fe23f9 Compare June 2, 2023 22:00

RogerHaase merged commit 0dcde9b into moinwiki:master Jun 3, 2023

UlrichB22 mentioned this pull request Apr 6, 2024

Switch to Sygil-Dev/whoosh-reloaded #1645

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repairing parentid for destroy rev, fixes #1448 #1455

repairing parentid for destroy rev, fixes #1448 #1455

bylsmad commented May 21, 2023

bylsmad May 21, 2023

RogerHaase commented May 21, 2023

bylsmad commented May 22, 2023

RogerHaase commented May 22, 2023

bylsmad commented May 23, 2023

RogerHaase commented May 23, 2023

RogerHaase commented May 28, 2023

bylsmad commented May 28, 2023

bylsmad commented May 29, 2023

bylsmad May 29, 2023

RogerHaase May 29, 2023

bylsmad May 29, 2023

RogerHaase commented May 29, 2023

bylsmad commented May 29, 2023

RogerHaase commented May 30, 2023

bylsmad commented May 31, 2023

RogerHaase commented May 31, 2023

bylsmad commented May 31, 2023

bylsmad commented Jun 1, 2023 •

edited

Loading

RogerHaase commented Jun 1, 2023

bylsmad commented Jun 2, 2023

bylsmad commented Jun 2, 2023

RogerHaase commented Jun 2, 2023

RogerHaase commented Jun 2, 2023

bylsmad commented Jun 2, 2023 •

edited

Loading

RogerHaase commented Jun 3, 2023

repairing parentid for destroy rev, fixes #1448 #1455

repairing parentid for destroy rev, fixes #1448 #1455

Conversation

bylsmad commented May 21, 2023

bylsmad May 21, 2023

Choose a reason for hiding this comment

RogerHaase commented May 21, 2023

bylsmad commented May 22, 2023

RogerHaase commented May 22, 2023

bylsmad commented May 23, 2023

RogerHaase commented May 23, 2023

RogerHaase commented May 28, 2023

bylsmad commented May 28, 2023

bylsmad commented May 29, 2023

bylsmad May 29, 2023

Choose a reason for hiding this comment

RogerHaase May 29, 2023

Choose a reason for hiding this comment

bylsmad May 29, 2023

Choose a reason for hiding this comment

RogerHaase commented May 29, 2023

bylsmad commented May 29, 2023

RogerHaase commented May 30, 2023

bylsmad commented May 31, 2023

RogerHaase commented May 31, 2023

bylsmad commented May 31, 2023

bylsmad commented Jun 1, 2023 • edited Loading

RogerHaase commented Jun 1, 2023

bylsmad commented Jun 2, 2023

bylsmad commented Jun 2, 2023

RogerHaase commented Jun 2, 2023

RogerHaase commented Jun 2, 2023

bylsmad commented Jun 2, 2023 • edited Loading

RogerHaase commented Jun 3, 2023

bylsmad commented Jun 1, 2023 •

edited

Loading

bylsmad commented Jun 2, 2023 •

edited

Loading