search

eXist-db / exist

eXist Native XML Database and Application Platform
https://exist-db.org
GNU Lesser General Public License v2.1
424 stars 179 forks source link

Fix PermissionDeniedException in NativeBroker.defragXMLresource #5311

Open line-o opened 4 months ago

line-o commented 4 months ago

Description:

Additional bugfix for NativeBroker.defragXMLresource where in specific setups with modules and collections with set group ID set a PermissionDeniedException is thrown when the modified XML resource is defragmented.

Example setup

Before this patch is applied the defragXMLResource method attempted to change the owner of the tempDoc which it cannot do as the effective subject is guest.

Reference:

refs #5273 refs #5276

Type of tests:

Code review was addressed in UpdateInsertTriggersDefrag.java

sonarcloud[bot] commented 4 months ago

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

line-o commented 4 months ago

@adamretter Please note that the test was adapted to address your review in #5273 and also note that the test was written based on src/test/java/org/exist/xquery/update/AbstractTestUpdate.java Any improvements in this family of tests for updates should be addressed separately. The seriousness of the problem fixed weighs higher in my opinion.

adamretter commented 4 months ago

Any improvements in this family of tests for updates should be addressed separately. The seriousness of the problem fixed weighs higher in my opinion.

I don't agree. Your code fixed one thing you were interested in but also breaks another thing that you are not interested in. That is not how changes should be implemented in collaborative project IMHO.

line-o commented 4 months ago

@adamretter What do you mean with this statement?

Your code fixed one thing you were interested in but also breaks another thing that you are not interested in.

What did the code break and which code are you talking about, the test?

line-o commented 4 months ago

Maybe let's look at this the other way around. What is the use of the call to DocumentImpl#copyOf here? @adamretter

wolfgangmm commented 4 months ago

@adamretter please watch your language, which I perceive as disrespectful in the last comments answering @line-o. We have no direct interest in this bugfix as we're not using any code affected by it. It's all just for the community's benefit.

adamretter commented 4 months ago

@wolfgangmm I will. No disrespect was or is intended. I'm sorry you felt that way. I have constantly written offers to help throughout this thread. I trust you have seen them? I think I've also made it clear that I'd like to see this fixed. It sounds like it could be important. But I want to see a fix to the problem and not to the symptom. To be doubly clear - I can't accept this PR when the problem (the root cause) is not understood.

wolfgangmm commented 4 months ago

@adamretter good. @line-o submitted the PR with the best intentions and did a great job reproducing this rather puzzling edge case.

In principle I agree the problem concerning permissions would be worth more investigation because it might also affect other copy operations. I was just arguing that it is not the correct solution for the corruption issue we try to address with this PR. The offending line was carried on over the years without checking if it still applies to the particular case of the defrag operation. As things are, it should clearly be dropped.

I'll try to provide more details concerning the other, the permission problem as time allows. It should be straightforward to reproduce using the setup instructions @line-o provided.

adamretter commented 4 months ago

@line-o submitted the PR with the best intentions

I have never questioned that, nor have I cast any doubt to the contrary.

I am simply following a scientific method without emotion to: (a) understand the problem, (b) locate the root of the problem, (c) propose a hypothesis for a fix, and (d) test and prove such a hypotheis through experimental evidence.

it is not the correct solution for the corruption issue we try to address with this PR

@wolfgangmm Perhaps I am misunderstanding something... My understanding is:

  1. copyOf sets the permissions of the tempDoc the same as the original doc.
  2. defrag throws a PermissionDeniedException; which only as a side-effect corrupts the database due to inconsistent state.
  3. you state that the issue is not within copyOf, but that there is a problem with permissions elsewhere whose symptoms are exhibited through the PermissionDeniedException.

Do I have that correct?

Assuming that I have understood correctly, then I don't understand how this PR fixes the real issue. Removing the call to copyOf doesn't solve the issue, rather it still feels to me that this is indeed a workaround, that if anything makes the situation more difficult as it hides the evidence of an underlying issue which before this PR we could of observed and therefore potentially addressed.

I see that there is a test added in this PR. If I was to run just that test without the changes in this PR against eXist-db 7.0.0-SNAPSHOT, would it show me the PermissionDeniedException? If so I can then use that to debug the underlying issue, and I will work forward on that basis. If not, then can you tell me what the test demonstrates?

line-o commented 4 months ago

@adamretter I am wondering how to put two recent quotes from you together:

last week in this PR:

Your code fixed one thing you were interested in but also breaks another thing that you are not interested in.

By the way, you still haven't answered what I broke, allegedly.

In Slack 3 days ago:

My general advice to my customers still with eXist-db is not to use XQuery Update, and to instead always overtime the complete document

So I fixed a thing users in our community are interested in: exist-db's XQuery update extension. You clearly stated that you are not interested in it. So what is your vested interest here, as this change only affects defragmentation which can only ever run after modification?

adamretter commented 4 months ago

By the way, you still haven't answered what I broke, allegedly.

Yes I did... it's been in the review comments of your PR for weeks now - as discussed there, your newly added tests are leaking resources.

My general advice to my customers still with eXist-db is not to use XQuery Update, and to instead always overtime the complete document

So I fixed a thing users in our community are interested in: exist-db's XQuery update extension. You clearly stated that you are not interested in it. So what is your vested interest here, as this change only affects defragmentation which can only ever run after modification?

I don't really see what you are getting at here! My interest is advising eXist-db users as to the best way of using eXist-db. Your fix addresses an issue with defragmentation, it does not fix other issues with XQuery Update.

line-o commented 4 months ago

OK, so then have a look at the test in this PR as it is changed to address your review.

it does not fix other issues with XQuery Update.

But it does fix an existing, reproducable and severe issue with XQuery Update. Other issues, you hinted at, are out of scope.

adamretter commented 4 months ago

OK, so then have a look at the test in this PR as it is changed to address your review.

I'm not seeing that in this PR.

line-o commented 4 months ago

Well it is the other changed file. And your review so far did only address the removal of the unnecessary call of copyOf. I would still like to hear why you think it cannot be removed.

adamretter commented 4 months ago

Well it is the other changed file.

No it isn't. It does not fully address my review and is still missing several things.

line-o commented 3 months ago

No it isn't. It does not fully address my review and is still missing several things.

I would need some more info on what you think is missing.