From d5947d4a37d441d3981af8651dc6e567d2aa8762 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Jul 2026 04:24:18 +0000 Subject: [PATCH 1/4] Simplify item ID resolution to drop module-prefixed lookup DivinityProvider previously namespaced item IDs by their owning module (e.g. "custom_items:foobar") when resolving items by ItemStack or ID string. Since item IDs are unique across modules in practice, this indirection added complexity without a real need; getItem()/getID() now resolve by plain item ID. Note for reviewers: this also drops the DivinityProviderTest cases that covered the namespaced-lookup behavior (getItem_namespacedIdIncludesModule, getItem_itemStackUsesStoredModule), since that behavior no longer exists. --- .../divinity/utils/DivinityProvider.java | 16 +----- .../divinity/utils/DivinityProviderTest.java | 51 +------------------ 2 files changed, 3 insertions(+), 64 deletions(-) diff --git a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java index 9a58d397..6eee5500 100644 --- a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java +++ b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java @@ -101,10 +101,6 @@ public DivinityItemType getItem(String id) { public DivinityProvider.DivinityItemType getItem(ItemStack itemStack) { String id = ItemStats.getId(itemStack); if (id == null) return null; - QModuleDrop module = ItemStats.getModule(itemStack); - if (module != null) { - id = module.getId() + ":" + id; - } return getItem(id); } @@ -118,15 +114,7 @@ public boolean isCustomItemOfId(ItemStack item, String id) { id = PrefixHelper.stripPrefix(NAMESPACE, id); String itemId = ItemStats.getId(item); - if (itemId == null) return false; - - String[] split = id.split(":", 2); - if (split.length < 2) { - return itemId.equals(id); - } - - QModuleDrop module = ItemStats.getModule(item); - return module != null && module.getId().equalsIgnoreCase(split[0]) && itemId.equals(split[1]); + return itemId != null && itemId.equals(id); } public static class DivinityItemType extends ItemType { @@ -158,7 +146,7 @@ public String getNamespace() { @Override public String getID() { - return this.moduleItem.getModule().getId() + ":" + this.moduleItem.getId(); + return this.moduleItem.getId(); } @Override diff --git a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java index 4e8712c3..f60dc867 100644 --- a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java +++ b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java @@ -5,17 +5,14 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.MockedStatic; -import org.bukkit.inventory.ItemStack; import studio.magemonkey.codex.Codex; import studio.magemonkey.codex.CodexEngine; import studio.magemonkey.codex.items.CodexItemManager; import studio.magemonkey.codex.modules.ModuleManager; import studio.magemonkey.divinity.Divinity; -import studio.magemonkey.divinity.modules.api.QModuleDrop; import studio.magemonkey.divinity.modules.list.arrows.ArrowManager; import studio.magemonkey.divinity.modules.list.customitems.CustomItemsManager; import studio.magemonkey.divinity.modules.list.itemgenerator.ItemGeneratorManager; -import studio.magemonkey.divinity.stats.items.ItemStats; import java.util.List; import java.util.logging.Logger; @@ -63,11 +60,9 @@ void setUp() { itemGenModule = spy(new ItemGeneratorManager(divinity)); when(moduleManager.getModule("item_generator")).thenReturn(itemGenModule); when(moduleManager.getModules()).thenReturn(List.of(arrowModule, itemGenModule)); - doReturn("item_generator").when(itemGenModule).getId(); customItemsModule = spy(new CustomItemsManager(divinity)); when(moduleManager.getModule("custom_items")).thenReturn(customItemsModule); - doReturn("custom_items").when(customItemsModule).getId(); //noinspection unchecked when(divinity.getModuleManager()).thenReturn(moduleManager); @@ -83,8 +78,6 @@ void afterEach() { void getItem_usesLevel() { ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); - when(generatorItem.getId()).thenReturn("foobar"); - doReturn((QModuleDrop) itemGenModule).when(generatorItem).getModule(); DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_item_generator:foobar~level:5"); @@ -100,8 +93,6 @@ void getItem_usesLevel() { void getItem_usesMaterial() { ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); - when(generatorItem.getId()).thenReturn("foobar"); - doReturn((QModuleDrop) itemGenModule).when(generatorItem).getModule(); DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_item_generator:foobar~material:VANILLA_DIAMOND"); @@ -119,8 +110,6 @@ void getItem_usesMaterial() { void getItem_noModule_returnsItem() { ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); - when(generatorItem.getId()).thenReturn("foobar"); - doReturn((QModuleDrop) itemGenModule).when(generatorItem).getModule(); DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_foobar"); @@ -136,8 +125,6 @@ void getItem_noModule_returnsItem() { void getItem_customItems_returnsItem() { CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); doReturn(codexItem).when(customItemsModule).getItemById("foobar"); - when(codexItem.getId()).thenReturn("foobar"); - doReturn((QModuleDrop) customItemsModule).when(codexItem).getModule(); DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_custom_items:foobar"); @@ -148,40 +135,4 @@ void getItem_customItems_returnsItem() { assertEquals(codexItem, item.getModuleItem()); assertInstanceOf(DivinityProvider.DivinityItemType.class, item); } - - @Test - void getItem_namespacedIdIncludesModule() { - CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); - doReturn(codexItem).when(customItemsModule).getItemById("foobar"); - when(codexItem.getId()).thenReturn("foobar"); - doReturn((QModuleDrop) customItemsModule).when(codexItem).getModule(); - - DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_custom_items:foobar"); - - assertNotNull(item); - assertEquals("DIVINITY_custom_items:foobar", item.getNamespacedID()); - } - - @Test - void getItem_itemStackUsesStoredModule() { - ItemStack itemStack = mock(ItemStack.class); - - ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); - doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); - - CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); - doReturn(codexItem).when(customItemsModule).getItemById("foobar"); - - try (MockedStatic itemStats = mockStatic(ItemStats.class)) { - itemStats.when(() -> ItemStats.getId(itemStack)).thenReturn("foobar"); - itemStats.when(() -> ItemStats.getModule(itemStack)).thenReturn(customItemsModule); - - DivinityProvider.DivinityItemType item = provider.getItem(itemStack); - - assertNotNull(item); - assertEquals(codexItem, item.getModuleItem()); - verify(customItemsModule).getItemById("foobar"); - verify(itemGenModule, never()).getItemById("foobar"); - } - } -} +} \ No newline at end of file From be7f123216fa4ee529c8a2055b908b90a21aa7c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 04:08:58 +0000 Subject: [PATCH 2/4] Keep isCustomItemOfId backward compatible with legacy namespaced IDs getItem(String) already parses the legacy "module:id" namespaced form unchanged, so external references captured via the old getID() format still resolve correctly. isCustomItemOfId did not have an equivalent fallback: it moved straight to plain-ID equality, so any caller still passing a namespaced id here (matching this method's previous contract) would always get false after the simplification. Fall back to the old module+split check when plain equality fails and the id looks namespaced, so existing callers aren't silently broken. --- .../magemonkey/divinity/utils/DivinityProvider.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java index 6eee5500..3bd8f534 100644 --- a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java +++ b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java @@ -114,7 +114,15 @@ public boolean isCustomItemOfId(ItemStack item, String id) { id = PrefixHelper.stripPrefix(NAMESPACE, id); String itemId = ItemStats.getId(item); - return itemId != null && itemId.equals(id); + if (itemId == null) return false; + if (itemId.equals(id)) return true; + + // Backward compatibility: older callers may still pass the legacy + // "module:id" namespaced form this method used to require. + String[] split = id.split(":", 2); + if (split.length < 2) return false; + QModuleDrop module = ItemStats.getModule(item); + return module != null && module.getId().equalsIgnoreCase(split[0]) && itemId.equals(split[1]); } public static class DivinityItemType extends ItemType { From ae47e7e3a4729250dfbe090fd0af7cedaa63350a Mon Sep 17 00:00:00 2001 From: Trav Date: Sat, 12 Sep 2026 12:18:47 -0600 Subject: [PATCH 3/4] Reject ambiguous bare item ids instead of picking one silently Bare-id lookup in DivinityProvider now checks all modules for a match: if exactly one module has the id, it resolves as before; if two or more modules share it, log an error naming the conflicting modules and return null instead of returning whichever module happened to iterate first. Module-prefixed "module:id" lookup is unaffected and remains the way to disambiguate. getItem(ItemStack) again prefers the item's stored module when present, so items resolve directly against their own module and skip the ambiguity check entirely. Restores the itemStack-uses-stored-module regression test dropped in d5947d4a and adds coverage for the new ambiguity handling. Co-Authored-By: Claude Sonnet 5 --- .../divinity/utils/DivinityProvider.java | 24 +++++- .../divinity/utils/DivinityProviderTest.java | 77 ++++++++++++++++++- 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java index 3bd8f534..1937948b 100644 --- a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java +++ b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java @@ -18,9 +18,12 @@ import studio.magemonkey.divinity.modules.list.itemgenerator.ItemGeneratorManager; import studio.magemonkey.divinity.stats.items.ItemStats; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; public class DivinityProvider implements ICodexItemProvider { public static final String NAMESPACE = "DIVINITY"; @@ -82,12 +85,23 @@ public DivinityItemType getItem(String id) { IModule module = Divinity.getInstance().getModuleManager().getModule(split[0]); if (!(module instanceof QModuleDrop)) return null; moduleItem = ((QModuleDrop) module).getItemById(split[1]); - } else { // Look in all modules + } else { // Look in all modules; require the id to be unambiguous + List> matches = new ArrayList<>(); for (IModule module : Divinity.getInstance().getModuleManager().getModules()) { if (!(module instanceof QModuleDrop)) continue; - moduleItem = ((QModuleDrop) module).getItemById(id); - if (moduleItem != null) break; + ModuleItem candidate = ((QModuleDrop) module).getItemById(id); + if (candidate != null) { + matches.add(module); + moduleItem = candidate; + } + } + + if (matches.size() > 1) { + Codex.error("Ambiguous Divinity item id '" + id + "' found in multiple modules (" + + matches.stream().map(IModule::getId).collect(Collectors.joining(", ")) + + "). Refer to it as ':" + id + "' to disambiguate."); + return null; } } @@ -101,6 +115,10 @@ public DivinityItemType getItem(String id) { public DivinityProvider.DivinityItemType getItem(ItemStack itemStack) { String id = ItemStats.getId(itemStack); if (id == null) return null; + QModuleDrop module = ItemStats.getModule(itemStack); + if (module != null) { + id = module.getId() + ":" + id; + } return getItem(id); } diff --git a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java index f60dc867..178a645d 100644 --- a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java +++ b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java @@ -1,6 +1,7 @@ package studio.magemonkey.divinity.utils; import org.bukkit.Material; +import org.bukkit.inventory.ItemStack; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -13,6 +14,7 @@ import studio.magemonkey.divinity.modules.list.arrows.ArrowManager; import studio.magemonkey.divinity.modules.list.customitems.CustomItemsManager; import studio.magemonkey.divinity.modules.list.itemgenerator.ItemGeneratorManager; +import studio.magemonkey.divinity.stats.items.ItemStats; import java.util.List; import java.util.logging.Logger; @@ -59,10 +61,13 @@ void setUp() { itemGenModule = spy(new ItemGeneratorManager(divinity)); when(moduleManager.getModule("item_generator")).thenReturn(itemGenModule); - when(moduleManager.getModules()).thenReturn(List.of(arrowModule, itemGenModule)); + doReturn("item_generator").when(itemGenModule).getId(); customItemsModule = spy(new CustomItemsManager(divinity)); when(moduleManager.getModule("custom_items")).thenReturn(customItemsModule); + doReturn("custom_items").when(customItemsModule).getId(); + + when(moduleManager.getModules()).thenReturn(List.of(arrowModule, itemGenModule, customItemsModule)); //noinspection unchecked when(divinity.getModuleManager()).thenReturn(moduleManager); @@ -135,4 +140,72 @@ void getItem_customItems_returnsItem() { assertEquals(codexItem, item.getModuleItem()); assertInstanceOf(DivinityProvider.DivinityItemType.class, item); } -} \ No newline at end of file + + @Test + void getItem_itemStackUsesStoredModule() { + ItemStack itemStack = mock(ItemStack.class); + + ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); + doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); + + CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); + doReturn(codexItem).when(customItemsModule).getItemById("foobar"); + + try (MockedStatic itemStats = mockStatic(ItemStats.class)) { + itemStats.when(() -> ItemStats.getId(itemStack)).thenReturn("foobar"); + itemStats.when(() -> ItemStats.getModule(itemStack)).thenReturn(customItemsModule); + + DivinityProvider.DivinityItemType item = provider.getItem(itemStack); + + assertNotNull(item); + assertEquals(codexItem, item.getModuleItem()); + verify(customItemsModule).getItemById("foobar"); + verify(itemGenModule, never()).getItemById("foobar"); + } + } + + @Test + void getItem_itemStackWithoutStoredModule_fallsBackToBareIdLookup() { + ItemStack itemStack = mock(ItemStack.class); + + ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); + doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); + + try (MockedStatic itemStats = mockStatic(ItemStats.class)) { + itemStats.when(() -> ItemStats.getId(itemStack)).thenReturn("foobar"); + itemStats.when(() -> ItemStats.getModule(itemStack)).thenReturn(null); + + DivinityProvider.DivinityItemType item = provider.getItem(itemStack); + + assertNotNull(item); + assertEquals(generatorItem, item.getModuleItem()); + } + } + + @Test + void getItem_ambiguousBareId_returnsNull() { + ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); + doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); + + CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); + doReturn(codexItem).when(customItemsModule).getItemById("foobar"); + + DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_foobar"); + + assertNull(item); + } + + @Test + void getItem_ambiguousId_stillResolvableWithModulePrefix() { + ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); + doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); + + CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); + doReturn(codexItem).when(customItemsModule).getItemById("foobar"); + + DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_custom_items:foobar"); + + assertNotNull(item); + assertEquals(codexItem, item.getModuleItem()); + } +} From 80fbfcf26be76b49d00ea10554e7d1e10f95c022 Mon Sep 17 00:00:00 2001 From: Trav Date: Sat, 12 Sep 2026 12:25:37 -0600 Subject: [PATCH 4/4] Make DivinityItemType.getID() module-prefixed when the id is ambiguous getID() previously always returned the plain item id, even when two modules shared that id (an ambiguous case getItem() already refuses to resolve without a module prefix). Now getID() checks the same ambiguity condition and only returns the plain id when it's unique across modules, prefixing with ":" otherwise so callers get a representation that's actually resolvable. Both getItem() and getID() now share findModuleItemsById(), which looks up the item once per module and keeps the (module -> item) association instead of doing a second identical lookup pass. Co-Authored-By: Claude Sonnet 5 --- .../divinity/utils/DivinityProvider.java | 41 ++++++++++++------- .../divinity/utils/DivinityProviderTest.java | 28 +++++++++++++ 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java index 1937948b..d994a50f 100644 --- a/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java +++ b/src/main/java/studio/magemonkey/divinity/utils/DivinityProvider.java @@ -18,8 +18,8 @@ import studio.magemonkey.divinity.modules.list.itemgenerator.ItemGeneratorManager; import studio.magemonkey.divinity.stats.items.ItemStats; -import java.util.ArrayList; -import java.util.List; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -86,23 +86,18 @@ public DivinityItemType getItem(String id) { if (!(module instanceof QModuleDrop)) return null; moduleItem = ((QModuleDrop) module).getItemById(split[1]); } else { // Look in all modules; require the id to be unambiguous - List> matches = new ArrayList<>(); - for (IModule module : Divinity.getInstance().getModuleManager().getModules()) { - if (!(module instanceof QModuleDrop)) continue; - - ModuleItem candidate = ((QModuleDrop) module).getItemById(id); - if (candidate != null) { - matches.add(module); - moduleItem = candidate; - } - } + Map, ModuleItem> matches = findModuleItemsById(id); if (matches.size() > 1) { Codex.error("Ambiguous Divinity item id '" + id + "' found in multiple modules (" - + matches.stream().map(IModule::getId).collect(Collectors.joining(", ")) + + matches.keySet().stream().map(IModule::getId).collect(Collectors.joining(", ")) + "). Refer to it as ':" + id + "' to disambiguate."); return null; } + + if (!matches.isEmpty()) { + moduleItem = matches.values().iterator().next(); + } } if (moduleItem != null) return new DivinityItemType(moduleItem, level, material); @@ -110,6 +105,20 @@ public DivinityItemType getItem(String id) { return null; } + /** + * Finds every module that has an item registered under the given plain id. + */ + private static Map, ModuleItem> findModuleItemsById(String id) { + Map, ModuleItem> matches = new LinkedHashMap<>(); + for (IModule module : Divinity.getInstance().getModuleManager().getModules()) { + if (!(module instanceof QModuleDrop)) continue; + + ModuleItem candidate = ((QModuleDrop) module).getItemById(id); + if (candidate != null) matches.put(module, candidate); + } + return matches; + } + @Override @Nullable public DivinityProvider.DivinityItemType getItem(ItemStack itemStack) { @@ -172,7 +181,11 @@ public String getNamespace() { @Override public String getID() { - return this.moduleItem.getId(); + String id = this.moduleItem.getId(); + if (findModuleItemsById(id).size() > 1) { + return this.moduleItem.getModule().getId() + ":" + id; + } + return id; } @Override diff --git a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java index 178a645d..ecea0070 100644 --- a/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java +++ b/src/test/java/studio/magemonkey/divinity/utils/DivinityProviderTest.java @@ -208,4 +208,32 @@ void getItem_ambiguousId_stillResolvableWithModulePrefix() { assertNotNull(item); assertEquals(codexItem, item.getModuleItem()); } + + @Test + void getID_uniqueId_returnsBareId() { + CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); + doReturn(codexItem).when(customItemsModule).getItemById("foobar"); + when(codexItem.getId()).thenReturn("foobar"); + + DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_custom_items:foobar"); + + assertNotNull(item); + assertEquals("foobar", item.getID()); + } + + @Test + void getID_ambiguousId_returnsModulePrefixedId() { + ItemGeneratorManager.GeneratorItem generatorItem = mock(ItemGeneratorManager.GeneratorItem.class); + doReturn(generatorItem).when(itemGenModule).getItemById("foobar"); + + CustomItemsManager.CustomItem codexItem = mock(CustomItemsManager.CustomItem.class); + doReturn(codexItem).when(customItemsModule).getItemById("foobar"); + when(codexItem.getId()).thenReturn("foobar"); + doReturn(customItemsModule).when(codexItem).getModule(); + + DivinityProvider.DivinityItemType item = provider.getItem("DIVINITY_custom_items:foobar"); + + assertNotNull(item); + assertEquals("custom_items:foobar", item.getID()); + } }