Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert IFile.create(...REPLACE) - fixes #1433 #1434

Merged
merged 4 commits into from
Jun 19, 2024
Merged

Revert IFile.create(...REPLACE) - fixes #1433 #1434

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6068480
Revert IFile.create(...REPLACE) - fixes #1433
EcljpseB0T Jun 18, 2024
1acb905
IFile.write: atomic operation including setDerived()
EcljpseB0T Jun 18, 2024
71b4cb2
A value of false will not update the derived flag of an existing file.
EcljpseB0T Jun 18, 2024
6825d24
do not touch FileSystemResourceManager
EcljpseB0T Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions ...rces/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public void create(InputStream content, int updateFlags, IProgressMonitor monito
final ISchedulingRule rule = workspace.getRuleFactory().createRule(this);
try {
workspace.prepareOperation(rule, subMonitor.newChild(1));
checkCreatable(updateFlags);
checkCreatable();
workspace.beginOperation(true);
IFileStore store = getStore();
IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store);
Expand Down Expand Up @@ -172,7 +172,7 @@ public void create(byte[] content, int updateFlags, IProgressMonitor monitor) th
final ISchedulingRule rule = workspace.getRuleFactory().createRule(this);
try {
workspace.prepareOperation(rule, subMonitor.newChild(1));
checkCreatable(updateFlags);
checkCreatable();
workspace.beginOperation(true);
IFileStore store = getStore();
IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store);
Expand Down Expand Up @@ -202,10 +202,8 @@ public void create(byte[] content, int updateFlags, IProgressMonitor monitor) th
}
}

private void checkCreatable(int updateFlags) throws CoreException {
if ((updateFlags & IResource.REPLACE) == 0) {
checkDoesNotExist();
}
private void checkCreatable() throws CoreException {
checkDoesNotExist();
Container parent = (Container) getParent();
ResourceInfo info = parent.getResourceInfo(false, false);
parent.checkAccessible(getFlags(info));
Expand All @@ -232,7 +230,7 @@ private IFileInfo create(int updateFlags, SubMonitor subMonitor, IFileStore stor
}
}
} else {
if (!BitMask.isSet(updateFlags, IResource.REPLACE) && localInfo.exists()) {
if (localInfo.exists()) {
// return an appropriate error message for case variant collisions
if (!Workspace.caseSensitive) {
String name = getLocalManager().getLocalName(store);
Expand Down
30 changes: 13 additions & 17 deletions resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/resources/IFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,13 +304,6 @@ public interface IFile extends IResource, IEncodedStorage, IAdaptable {
* with a value of <code>true</code> immediately after creating the resource.
* </p>
* <p>
* The {@link IResource#KEEP_HISTORY} update flag indicates that the history of this file is kept if any
* </p>
* <p>
* The {@link IResource#REPLACE} update flag indicates that this resource is overridden if already existing.
*
* </p>
* <p>
* The {@link IResource#TEAM_PRIVATE} update flag indicates that this resource
* should immediately be set as a team private resource. Specifying this flag
* is equivalent to atomically calling {@link IResource#setTeamPrivateMember(boolean)}
Expand Down Expand Up @@ -338,9 +331,7 @@ public interface IFile extends IResource, IEncodedStorage, IAdaptable {
* @param source an input stream containing the initial contents of the file,
* or <code>null</code> if the file should be marked as not local
* @param updateFlags bit-wise or of update flag constants
* ({@link IResource#FORCE}, {@link IResource#DERIVED},
* {@link IResource#KEEP_HISTORY}, {@link IResource#REPLACE},
* and {@link IResource#TEAM_PRIVATE})
* ({@link IResource#FORCE}, {@link IResource#DERIVED}, and {@link IResource#TEAM_PRIVATE})
* @param monitor a progress monitor, or <code>null</code> if progress
* reporting is not desired
* @exception CoreException if this method fails. Reasons include:
Expand Down Expand Up @@ -400,9 +391,7 @@ public default void create(byte[] content, boolean force, boolean derived, IProg
*
* @param content the content as byte array
* @param createFlags bit-wise or of flag constants ({@link IResource#FORCE},
* {@link IResource#DERIVED},
* {@link IResource#KEEP_HISTORY},
* {@link IResource#REPLACE}, and
* {@link IResource#DERIVED}, and
* {@link IResource#TEAM_PRIVATE})
* @param monitor a progress monitor, or <code>null</code> if progress
* reporting is not desired
Expand All @@ -416,8 +405,9 @@ public default void create(byte[] content, int createFlags, IProgressMonitor mon
/**
* Creates the file and sets the file content. If the file already exists in
* workspace its content is replaced. Shortcut for calling
* {@link #create(byte[], int, IProgressMonitor)} with flags depending on the
* boolean parameters given and {@link IResource#REPLACE}.
* {@link #setContents(byte[], boolean, boolean, IProgressMonitor)} or
* {@link #create(byte[], boolean, boolean, IProgressMonitor)} if the file does
* not {@link #exists()}.
*
* @param content the new content bytes. Must not be null.
* @param force a flag controlling how to deal with resources that are not
Expand All @@ -438,8 +428,14 @@ public default void create(byte[] content, int createFlags, IProgressMonitor mon
public default void write(byte[] content, boolean force, boolean derived, boolean keepHistory,
IProgressMonitor monitor) throws CoreException {
Objects.requireNonNull(content);
create(content, (force ? IResource.FORCE : 0) | (derived ? IResource.DERIVED : 0)
| (keepHistory ? IResource.KEEP_HISTORY : 0) | IResource.REPLACE, monitor);
if (exists()) {
if (derived != isDerived()) {
setDerived(derived, null);
}
setContents(content, force, keepHistory, monitor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, methods in resource are supposed to be atomic (ie grouped in a single operation), here those 2 methods would lead to 2 operations side by side but not grouped.
What you can try to remediate that would be something like

if (derived == isDerived()) {
	setContents(...);
} else {
	SubMonitor progress = SubMonitor.convert(monitor, 100).checkCanceled();
	// need to group operation
	final ISchedulingRule groupRule = new MultiRule[] { workspace.getRuleFactory().derivedRule(this), workspace.getRuleFactory().modifyRule(this) };
	try {
		workspace.prepareOperation(rule, progress.split(1)); // locks
		workspace.beginOperation(true);
		setDerived(derived, progress.split(1));
		setContents(content, force, keepHistory, progress.split(98))
	} catch (OperationCanceledException e) {
		workspace.getWorkManager().operationCanceled();
		throw e;
	} finally {
		progress.done();
		workspace.endOperation(rule, true); // this is extremely important of workspace is locked
	}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API-wise it's totally fine. See nested comment about the workspace operation should be atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I have tried your implementation and it would work but can not be done in default method of IFile due to references to internal methods. but based on the idea i think i have found a easier solution with less overhead. please review again. Junit also tests for atomic operations now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in default method of IFile due to references to internal methods.

There is nothing mandating to create a default method here. IFile is annotated as "noextends/noimplements", so we can add new interface methods in here and have the implementation in File.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far the therory. In practice there are other implementations even within the IDE (PDE's ContributionDataFile) :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's an annoyance, I think keeping the resource code as maintainable as posible is more important than keeping e4 tools alive. IMO, it's best to go for a regular interface method and impl in File, and to adapt PDE then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, that would be good. But i have no idea why it is like that in PDE and how it could be solved. So as long as we can implement a default its much easier to proceed.

} else {
create(content, force, derived, monitor);
}
}

/**
Expand Down
20 changes: 20 additions & 0 deletions ...ests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/IFileTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@
import org.eclipse.core.resources.IResourceChangeEvent;
import org.eclipse.core.resources.IResourceDelta;
import org.eclipse.core.resources.IResourceStatus;
import org.eclipse.core.resources.IWorkspace;
import org.eclipse.core.resources.IWorkspaceDescription;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.IProgressMonitor;
Expand Down Expand Up @@ -514,6 +516,24 @@ public void testWrite() throws CoreException {
assertEquals(keepHistory ? 1 : 0, history2.length - history1.length);
}
}

@Test
public void testWriteRule() throws CoreException {
IFile resource = projects[0].getFile("derived.txt");
createInWorkspace(projects[0]);
IWorkspace workspace = ResourcesPlugin.getWorkspace();
resource.delete(false, null);
workspace.run(pm -> {
resource.write(("create").getBytes(), false, false, false, null);
}, workspace.getRuleFactory().createRule(resource), IWorkspace.AVOID_UPDATE, null);
assertTrue(resource.exists());
// test that modifyRule can be used for IFile.write() if the file already exits:
workspace.run(pm -> {
resource.write(("replace").getBytes(), false, false, false, null);
}, workspace.getRuleFactory().modifyRule(resource), IWorkspace.AVOID_UPDATE, null);
assertTrue(resource.exists());
}

@Test
public void testDeltaOnCreateDerived() throws CoreException {
IFile derived = projects[0].getFile("derived.txt");
Expand Down
Loading