Skip to content

Stability update of v2.6.1 #170

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

Open
beefcake123 wants to merge 1 commit into
base: 2.6.1
Choose a base branch
from
Open

Stability update of v2.6.1 #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions pom.xml
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Want to say no because it potentially breaks compatibility with older versions, but nothings working anyways so 🤷

Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
<configuration>
<source>1.8</source>
<target>1.8</target>
<source>17</source>
<target>17</target>
</configuration>
</plugin>
<plugin>
Expand Down Expand Up @@ -257,7 +257,7 @@
<dependency>
<groupId>org.spigotmc</groupId>
<artifactId>spigot-api</artifactId>
<version>1.19.2-R0.1-SNAPSHOT</version>
<version>1.21.10-R0.1-SNAPSHOT</version>
<scope>provided</scope>
</dependency>

Expand Down
6 changes: 6 additions & 0 deletions src/main/java/org/shanerx/tradeshop/commands/commandrunners/ShopUserSubCommand.java
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if a null check here is necessary, but it doesn't change much either way.

If this is going to stay, line 106 needs to be changed to use the set target variable. No point retrieving the offline player twice...

Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ public void editUser(ShopRole role, ShopChange change) {
Set<Shop> ownedShops = new HashSet<>();
Map<String, String> updateStatuses = new HashMap<>();

target = Bukkit.getOfflinePlayer(command.getArgAt(1));
if(target == null) {
Message.PLAYER_NOT_FOUND.sendMessage(command.getPlayerSender());
return;
}

Shop tempShop = shopUserCommandStart(Bukkit.getOfflinePlayer(command.getArgAt(1)), applyAllOwned);

if (applyAllOwned) {
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/org/shanerx/tradeshop/data/storage/DataStorage.java
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of generic exception catches, especially in a for loop, but as long as it runs I think this is fine

Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
import org.shanerx.tradeshop.utils.Utils;
import org.shanerx.tradeshop.utils.debug.DebugLevels;

import com.google.gson.JsonParseException;
import com.google.gson.stream.MalformedJsonException;

import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -108,7 +111,18 @@ public int getShopCountInWorld(World world) {
if (folder.exists() && folder.listFiles() != null) {
for (File file : folder.listFiles()) {
if (file.getName().contains(world.getName()))
try {
count += new JsonShopConfiguration(ShopChunk.deserialize(file.getName().replace(".json", ""))).size();
} catch(Exception ex){
if(ex instanceof MalformedJsonException)
{
TradeShop.getPlugin().getLogger().warning("[REINARD] - Malformed JSON in file: " + file.getName());
}
else
{
TradeShop.getPlugin().getLogger().warning("[REINARD] - Error during loading shop" + file.getName() + ": " + ex.getClass().toString() + ": " + ex.getMessage() + ": " + ex.getCause());
}
}
}
}
break;
Expand Down
115 changes: 108 additions & 7 deletions src/main/java/org/shanerx/tradeshop/data/storage/Json/JsonConfiguration.java
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like these changes, as long as they've been tested and work

Great comments!

Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.util.logging.Level;

class JsonConfiguration extends Utils {
Expand All @@ -44,6 +47,7 @@ class JsonConfiguration extends Utils {
protected JsonObject jsonObj;

private final TradeShop PLUGIN = TradeShop.getPlugin();
private static final String TEMP_FOLDER_NAME = ".tmp";

/**
* Creates a JsonConfiguration object assisting with managing JSON data
Expand All @@ -68,6 +72,18 @@ public static File getPath(String folderFromData) {
return new File(TradeShop.getPlugin().getDataFolder().getAbsolutePath() + File.separator + "Data" + File.separator + folderFromData);
}

/**
* Gets the temporary folder for storing temporary files during writes.
* Creates the folder if it doesn't exist.
*/
private File getTempFolder() {
File tempFolder = new File(TradeShop.getPlugin().getDataFolder().getAbsolutePath() + File.separator + TEMP_FOLDER_NAME);
if (!tempFolder.exists()) {
tempFolder.mkdirs();
}
return tempFolder;
}

private void buildFilePath() {
try {
pathFile.mkdirs();
Expand All @@ -78,25 +94,110 @@ private void buildFilePath() {
}

protected void loadFile() {
try {
jsonObj = new JsonParser().parse(new FileReader(file)).getAsJsonObject();
try (FileReader fileReader = new FileReader(file)) {
jsonObj = new JsonParser().parse(fileReader).getAsJsonObject();
} catch (FileNotFoundException e) {
PLUGIN.getLogger().log(Level.SEVERE, "Could not load " + file.getName() + " file! Data may be lost!", e);
jsonObj = new JsonObject();
} catch (IllegalStateException e) {
jsonObj = new JsonObject();
} catch (IOException e) {
PLUGIN.getLogger().log(Level.SEVERE, "Could not read " + file.getName() + " file!", e);
jsonObj = new JsonObject();
}
}

/**
* Saves the JSON object to file using atomic write operation.
* This prevents data corruption by writing to a temporary file first,
* then atomically replacing the original file.
*
* This approach resolves issues with enchanted books and other complex
* serializable items that may cause partial writes or file corruption.
*/
protected void saveFile() {
String str = gson.toJson(jsonObj);
if (!str.isEmpty()) {
try {
FileWriter fileWriter = new FileWriter(this.file);
if (str.isEmpty()) {
return;
}

File tempFile = null;
try {
// Create temporary file in a separate .tmp folder
// This prevents the temporary files from interfering with deserialization logic
File tempFolder = getTempFolder();
tempFile = File.createTempFile(file.getName() + "_", ".tmp", tempFolder);

// Write data to temporary file
try (FileWriter fileWriter = new FileWriter(tempFile, StandardCharsets.UTF_8)) {
fileWriter.write(str);
fileWriter.flush();
fileWriter.close();
}

// Verify the temporary file was written correctly before replacing
if (tempFile.length() == 0) {
PLUGIN.getLogger().log(Level.SEVERE, "Failed to write to temporary file for " + file.getName() + "! File may be corrupted.");
if (tempFile.delete()) {
PLUGIN.getLogger().log(Level.INFO, "Cleaned up empty temporary file: " + tempFile.getPath());
}
return;
}

// Attempt to replace the file with retry logic for Windows file locking
replaceFileWithRetry(tempFile, file);

} catch (IOException e) {
PLUGIN.getLogger().log(Level.SEVERE, "Could not save " + file.getName() + " file! Data may be lost!", e);

// Cleanup temporary file on error
if (tempFile != null && tempFile.exists()) {
if (!tempFile.delete()) {
PLUGIN.getLogger().log(Level.WARNING, "Failed to cleanup temporary file: " + tempFile.getPath());
}
}
}
}

/**
* Attempts to replace the target file with the temporary file, with retry logic.
* This handles Windows file locking issues where files may be temporarily in use.
*/
private void replaceFileWithRetry(File tempFile, File targetFile) {
final int MAX_RETRIES = 3;
final long RETRY_DELAY_MS = 100;

for (int attempt = 1; attempt <= MAX_RETRIES; attempt++) {
try {
// Try atomic move first
try {
Files.move(tempFile.toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
return; // Success
} catch (IOException atomicMoveException) {
// Atomic move failed, try regular move (non-atomic)
try {
Files.move(tempFile.toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
return; // Success
} catch (IOException regularMoveException) {
// Both methods failed, throw to retry logic
throw regularMoveException;
}
}

} catch (IOException e) {
PLUGIN.getLogger().log(Level.SEVERE, "Could not save " + file.getName() + " file! Data may be lost!", e);
// If this is the last attempt, log error and keep temp file
if (attempt == MAX_RETRIES) {
PLUGIN.getLogger().log(Level.SEVERE, "Could not save " + targetFile.getName() + " file after " + MAX_RETRIES + " attempts! Temp file available at: " + tempFile.getAbsolutePath(), e);
return;
}

// Wait before retrying (gives file handles time to release)
try {
Thread.sleep(RETRY_DELAY_MS);
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
PLUGIN.getLogger().log(Level.WARNING, "File save retry interrupted for " + targetFile.getName());
return;
}
}
}
}
Expand Down
12 changes: 3 additions & 9 deletions src/main/java/org/shanerx/tradeshop/shop/Shop.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ public void setShopSettings(Map<ShopSettingKeys, ObjectHolder<?>> newSettings) {
* @return list of ShopUsers.
*/
public List<ShopUser> getUsers(ShopRole... roles) {
return getUsersExcluding(Collections.emptyList(), roles);
return getUsersUUID(roles).stream().map((uuid) -> new ShopUser(uuid, ShopRole.MANAGER)).collect(Collectors.toList());

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

God it's been awhile...

Pretty sure Users and Managers are separate lists and players shouldn't exist in both lists, hence the empty list.

I guess double check that and correct me if I'm wrong.

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll just correct myself;

Users refers to any users from Members, Managers, and Owner.

Your changes seem to force all users into Managers, which would be a problem.

I'm also not a fan of iterating through the list twice if we need an excluded list. Would prefer this method is left as it was and your changes be focused on fixing the getUsersExcluding method (Which seems to be recursive with what I can see here)

}

/**
Expand All @@ -713,13 +713,7 @@ public List<ShopUser> getUsers(ShopRole... roles) {
* @return list of ShopUsers.
*/
public List<ShopUser> getUsersExcluding(List<UUID> excludedPlayers, ShopRole... roles) {
List<ShopUser> users = new ArrayList<>();
getUsers(roles).forEach(user -> {
if (!excludedPlayers.contains(user.getUUID()))
users.add(user);
});

return users;
return getUsers(roles).stream().filter((shopUser) -> !excludedPlayers.contains(shopUser.getUUID())).collect(Collectors.toList());

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See comment on 705

}

/**
Expand Down Expand Up @@ -936,7 +930,7 @@ public void fixSide(ShopItemSide side) {

Set<Material> matSet = new HashSet<>();

ogItems.forEach((item) -> matSet.add(item.getItemStack().getType()));
ogItems.stream().filter(shopItemStack -> shopItemStack.getItemStack() != null).forEach((item) -> matSet.add(item.getItemStack().getType()));

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure how filter processes, if it processes the entire list before forEach then I don't like this at it would cause us to iterate the list twice. If it checks the filter before running the forEach on each item, then this is a good change.



if (ogItems.size() > 1 && ogItems.size() != matSet.size()) {
Expand Down
94 changes: 68 additions & 26 deletions src/main/java/org/shanerx/tradeshop/shop/ShopChest.java
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't have time right now to understand these changes, but a couple things to double check.

  1. This code might be run from hopper code, in which case any excessive try/catches should be avoided.
  2. We shouldn't be adding in commented debug code
  3. If this code is run from the hopper checks (which some of the old comments seem to make me think it is) There should be no debug code unless ABSOLUTELY necessary.
  4. A lot of your code seems to replace commented code, make sure this is truly needed. Hoppers being involved multiplies the time this runs so even if it takes nano-seconds to run it could potentially be run 10s of thousands of times a second.

Original file line number Diff line number Diff line change
Expand Up @@ -76,41 +76,83 @@ public ShopChest(Block chest, UUID owner, Location sign) {

public static boolean isShopChest(Block checking) {
try {
// Check if input is valid
if (checking == null) {
plugin.getDebugger().log("isShopChest: Block input is null", DebugLevels.PROTECTION);
return false;
}

if (isDoubleChest(checking)) {
//plugin.getDebugger().log("isShopChest: Block is a double chest", DebugLevels.PROTECTION);
DoubleChest dbl = getDoubleChest(checking);

return ((Container) dbl.getLeftSide()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING) ||
((Container) dbl.getRightSide()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING);
if (dbl == null) {
plugin.getDebugger().log("isShopChest: getDoubleChest returned null", DebugLevels.PROTECTION);
return false;
}

// Check left side
InventoryHolder leftSide = dbl.getLeftSide();
if (leftSide == null) {
//plugin.getDebugger().log("isShopChest: Double chest left side is null", DebugLevels.PROTECTION);
} else {
Container leftContainer = (Container) leftSide;
if (leftContainer.getPersistentDataContainer() == null) {
//plugin.getDebugger().log("isShopChest: Left container persistent data container is null", DebugLevels.PROTECTION);
} else if (leftContainer.getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING)) {
//plugin.getDebugger().log("isShopChest: Found sign key in left side of double chest", DebugLevels.PROTECTION);
return true;
}
}

// Check right side
InventoryHolder rightSide = dbl.getRightSide();
if (rightSide == null) {
//plugin.getDebugger().log("isShopChest: Double chest right side is null", DebugLevels.PROTECTION);
} else {
Container rightContainer = (Container) rightSide;
if (rightContainer.getPersistentDataContainer() == null) {
//plugin.getDebugger().log("isShopChest: Right container persistent data container is null", DebugLevels.PROTECTION);
} else if (rightContainer.getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING)) {
//plugin.getDebugger().log("isShopChest: Found sign key in right side of double chest", DebugLevels.PROTECTION);
return true;
}
}

return false;
}
boolean conHas = ((Container) checking.getState()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING);
return conHas;
} catch (NullPointerException | ClassCastException ex) {
plugin.getDebugger().log("NPE thrown during isShopChest by: \n" + ex.getCause(), DebugLevels.PROTECTION);
}
return false;

// Old MoveEvent Est ~50mspt
/*plugin.getDebugger().log("isShopChest checking Block at " + new ShopLocation(checking.getLocation()).serialize() + "", DebugLevels.PROTECTION);
try {
if (isDoubleChest(checking)) {
DoubleChest dbl = getDoubleChest(checking);
boolean leftHas = ((Container) dbl.getLeftSide()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING),
rightHas = ((Container) dbl.getRightSide()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING);
// Single chest check
//plugin.getDebugger().log("isShopChest: Block is a single inventory", DebugLevels.PROTECTION);
BlockState state = checking.getState();
if (state == null) {
plugin.getDebugger().log("isShopChest: Block state is null", DebugLevels.PROTECTION);
return false;
}

plugin.getDebugger().log("Block is DoubleChest", DebugLevels.PROTECTION);
plugin.getDebugger().log("Left side PerData: " + (leftHas ? ((Container) dbl.getLeftSide()).getPersistentDataContainer().get(plugin.getSignKey(), PersistentDataType.STRING) : "null"), DebugLevels.PROTECTION);
plugin.getDebugger().log("Right side PerData: " + (rightHas ? ((Container) dbl.getRightSide()).getPersistentDataContainer().get(plugin.getSignKey(), PersistentDataType.STRING) : "null"), DebugLevels.PROTECTION);
if (!(state instanceof Container)) {
plugin.getDebugger().log("isShopChest: Block state is not a Container: " + state.getType().toString(), DebugLevels.PROTECTION);
return false;
}

return leftHas || rightHas;
Container container = (Container) state;
if (container.getPersistentDataContainer() == null) {
plugin.getDebugger().log("isShopChest: Container persistent data container is null", DebugLevels.PROTECTION);
return false;
}
boolean conHas = ((Container) checking.getState()).getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING);
plugin.getDebugger().log("Block is SINGLE inventory", DebugLevels.PROTECTION);
plugin.getDebugger().log("Storage Block PerData: " + (conHas ? ((Container) checking.getState()).getPersistentDataContainer().get(plugin.getSignKey(), PersistentDataType.STRING) : "null"), DebugLevels.PROTECTION);
return conHas;
} catch (NullPointerException | ClassCastException ex) {
plugin.getDebugger().log("NPE thrown during isShopChest by: \n" + ex.getCause(), DebugLevels.PROTECTION);

boolean conHas = container.getPersistentDataContainer().has(plugin.getSignKey(), PersistentDataType.STRING);
//plugin.getDebugger().log("isShopChest: Single chest sign key check result: " + conHas, DebugLevels.PROTECTION);
return conHas;

} catch (NullPointerException ex) {
plugin.getDebugger().log("NPE thrown during isShopChest: " + ex.getMessage(), DebugLevels.PROTECTION);
plugin.getDebugger().log("Stack trace: ", DebugLevels.PROTECTION);
ex.printStackTrace();
} catch (ClassCastException ex) {
plugin.getDebugger().log("ClassCastException thrown during isShopChest: " + ex.getMessage(), DebugLevels.PROTECTION);
}
return false;*/
return false;
}

public static boolean isShopChest(Inventory checking) {
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/org/shanerx/tradeshop/shop/listeners/ShopCreateListener.java
View file
Open in desktop

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See line comments

Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.shanerx.tradeshop.shop.Shop;
import org.shanerx.tradeshop.shop.ShopType;
import org.shanerx.tradeshop.utils.Utils;
import org.shanerx.tradeshop.utils.debug.DebugLevels;

@SuppressWarnings("unused")
public class ShopCreateListener extends Utils implements Listener {
Expand All @@ -55,10 +56,15 @@ public void onSignChange(SignChangeEvent event) {
shopSign.setLine(3, event.getLine(3));

if (!ShopType.isShop(shopSign)) {
TradeShop.getPlugin().getDebugger().log("ShopType.isShop() returned false for sign: " + event.getLine(0), DebugLevels.SHOP_CREATION);

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe I omitted the debug logging on this as it'll cause an excessive amount of spam and unnecessary strain. This should be removed, if we need debugging for specific check conditions, then that can probably be added in #isShop.

return;
}

ShopType shopType = ShopType.getType(shopSign);
if (shopType == null) {
TradeShop.getPlugin().getDebugger().log("ShopType.getType() returned null for sign: " + event.getLine(0), DebugLevels.SHOP_CREATION);

@KillerOfPie KillerOfPie Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

50/50 on this one. This case should only come up if a shop is corrupted (I think, might be wrong), So I guess debug is fine, but the issue should probably be handled instead/as well.

return;
}
Player p = event.getPlayer();

// Clear the first line since we already know it is going to be a Shop, and we have the type to pass separately
Expand Down
Loading