-
Notifications
You must be signed in to change notification settings - Fork 0
fix(mongo): disable implicit player document initialization #8
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.
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
fix(mongo): disable implicit player document initialization #8
Conversation
…kDataAPI Prevent automatic creation of default MongoDB documents on player join. NetworkDataAPI should only expose a shared MongoDB connection layer, without managing or provisioning plugin data. Removed implicit initialization logic so custom plugins retain full control over their own schemas and document creation.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors NetworkDataAPI to be a pure MongoDB connection layer rather than an automatic player data manager. The changes remove implicit player document initialization, eliminate automatic event listener registration, and significantly expand documentation to clarify proper usage patterns for plugin developers.
- Removes automatic player data creation and listener registration
- Updates documentation to emphasize NetworkDataAPI's role as a shared connection pool
- Adds comprehensive implementation notes and best practice guidance
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| networkdataapi-core/src/main/java/com/cynive/networkdataapi/core/service/PlayerDataService.java | Removes createDefaultPlayerData() method and returns empty document instead of creating default player data |
| networkdataapi-paper/src/main/java/com/cynive/networkdataapi/paper/NetworkDataAPI.java | Removes automatic registration of PlayerConnectionListener |
| networkdataapi-paper/src/main/java/com/cynive/networkdataapi/paper/PlayerConnectionListener.java | Updates documentation to clarify this is an example implementation, not automatically registered |
| networkdataapi-bungee/src/main/java/com/cynive/networkdataapi/bungee/NetworkDataAPI.java | Removes automatic registration of PlayerConnectionListener |
| networkdataapi-bungee/src/main/java/com/cynive/networkdataapi/bungee/PlayerConnectionListener.java | Updates documentation to clarify this is an example implementation for Bungee |
| README.md | Extensively updated to explain NetworkDataAPI as a connection layer with real-world examples and best practices |
| IMPLEMENTATION_NOTES.md | New comprehensive document detailing design philosophy, usage patterns, and implementation details |
| EXAMPLE_PLUGIN_GUIDE.md | Updated to demonstrate correct usage as a database connection layer |
| API_DOCUMENTATION.md | Expanded with detailed examples for custom database/collection management and player event handling |
Comments suppressed due to low confidence (1)
networkdataapi-core/src/main/java/com/cynive/networkdataapi/core/service/PlayerDataService.java:130
- Caching empty documents for non-existent players may cause performance issues. When
getPlayerData()is called for a player that doesn't exist in the database, an empty document with only_idis created and cached. This means:
- Subsequent calls for the same UUID will get the cached empty document instead of checking if data was created by another plugin
- Multiple plugins calling
getPlayerData()will all get the same cached empty document, preventing them from seeing each other's data until cache expires
Consider either:
- Not caching documents that don't exist in the database
- Using a shorter TTL for empty documents
- Invalidating cache when
savePlayerData()is called
// Cache the result
cacheManager.put(cacheKey, data);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ❌ WRONG: Create empty data on player join | ||
| @EventHandler | ||
| public void onPlayerJoin(PlayerJoinEvent event) { | ||
| Document data = new Document("uuid", uuid).append("coins", 0); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable uuid is used but not defined in this code snippet. It should be:
UUID uuid = player.getUniqueId();
Document data = new Document("uuid", uuid.toString()).append("coins", 0);| Document data = new Document("uuid", uuid).append("coins", 0); | |
| UUID uuid = event.getPlayer().getUniqueId(); | |
| Document data = new Document("uuid", uuid.toString()).append("coins", 0); |
| Document data = new Document("uuid", uuid).append("coins", 0); | ||
| collection.insertOne(data); // Why create empty data? | ||
| } | ||
|
|
||
| // ✅ CORRECT: Create data when relevant | ||
| public void claimCosmetic(Player player, String cosmeticId) { | ||
| Document data = collection.find(Filters.eq("uuid", uuid)).first(); | ||
| if (data == null) { | ||
| // First cosmetic - NOW create the document | ||
| data = new Document("uuid", uuid).append("cosmetics", List.of(cosmeticId)); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable uuid is used but not defined in this code snippet. It should be:
UUID uuid = player.getUniqueId();
Document data = collection.find(Filters.eq("uuid", uuid.toString())).first();| Document data = new Document("uuid", uuid).append("coins", 0); | |
| collection.insertOne(data); // Why create empty data? | |
| } | |
| // ✅ CORRECT: Create data when relevant | |
| public void claimCosmetic(Player player, String cosmeticId) { | |
| Document data = collection.find(Filters.eq("uuid", uuid)).first(); | |
| if (data == null) { | |
| // First cosmetic - NOW create the document | |
| data = new Document("uuid", uuid).append("cosmetics", List.of(cosmeticId)); | |
| UUID uuid = event.getPlayer().getUniqueId(); | |
| Document data = new Document("uuid", uuid.toString()).append("coins", 0); | |
| collection.insertOne(data); // Why create empty data? | |
| } | |
| // ✅ CORRECT: Create data when relevant | |
| public void claimCosmetic(Player player, String cosmeticId) { | |
| UUID uuid = player.getUniqueId(); | |
| Document data = collection.find(Filters.eq("uuid", uuid.toString())).first(); | |
| if (data == null) { | |
| // First cosmetic - NOW create the document | |
| data = new Document("uuid", uuid.toString()).append("cosmetics", List.of(cosmeticId)); |
| playerData.savePlayerDataAsync(uuid, data); | ||
| } | ||
|
|
||
| // Update last login | ||
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling updateFieldAsync() on a new player will fail because the document doesn't exist in the database. Since getPlayerData() no longer creates a default document in the database, updateField() (which uses updateOne() without upsert) will not create the document, causing the update to silently fail.
The code should save the document before attempting to update it:
playerData.getPlayerDataAsync(uuid).thenAccept(data -> {
// Handle player data - set defaults if needed
if (!data.containsKey("myPluginData")) {
data.put("myPluginData", new Document()
.append("coins", 0)
.append("level", 1)
.append("firstJoin", System.currentTimeMillis())
);
data.put("lastLogin", System.currentTimeMillis());
playerData.savePlayerDataAsync(uuid, data);
} else {
// Update last login for existing players
playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis());
}
});| playerData.savePlayerDataAsync(uuid, data); | |
| } | |
| // Update last login | |
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | |
| data.put("lastLogin", System.currentTimeMillis()); | |
| playerData.savePlayerDataAsync(uuid, data).thenRun(() -> { | |
| // Update last login after document is saved | |
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | |
| }); | |
| } else { | |
| // Update last login for existing players | |
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | |
| } |
| Document data = collection.find(Filters.eq("uuid", uuid)).first(); | ||
| if (data == null) { | ||
| // First cosmetic - NOW create the document | ||
| data = new Document("uuid", uuid).append("cosmetics", List.of(cosmeticId)); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable uuid should be converted to String when storing in the document:
data = new Document("uuid", uuid.toString()).append("cosmetics", List.of(cosmeticId));| data = new Document("uuid", uuid).append("cosmetics", List.of(cosmeticId)); | |
| data = new Document("uuid", uuid.toString()).append("cosmetics", List.of(cosmeticId)); |
| playerData.savePlayerDataAsync(uuid, data); | ||
| } | ||
|
|
||
| // Update login timestamp | ||
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | ||
| playerData.updateFieldAsync(uuid, "lastKnownName", player.getName()); | ||
|
|
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling updateFieldAsync() on a new player will fail because the document doesn't exist in the database yet. Since getPlayerData() no longer creates a default document in the database, updateField() (which uses updateOne() without upsert) will not create the document.
This example should save the document first if it doesn't exist:
playerData.getPlayerDataAsync(uuid).thenAccept(data -> {
// Initialize default data if this is a new player
if (!data.containsKey("myPlugin")) {
data.put("myPlugin", new Document()
.append("coins", 0)
.append("level", 1)
.append("firstJoin", System.currentTimeMillis())
);
data.put("lastLogin", System.currentTimeMillis());
data.put("lastKnownName", player.getName());
playerData.savePlayerDataAsync(uuid, data);
} else {
// Update login timestamp for existing players
playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis());
playerData.updateFieldAsync(uuid, "lastKnownName", player.getName());
}
});| playerData.savePlayerDataAsync(uuid, data); | |
| } | |
| // Update login timestamp | |
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | |
| playerData.updateFieldAsync(uuid, "lastKnownName", player.getName()); | |
| data.put("lastLogin", System.currentTimeMillis()); | |
| data.put("lastKnownName", player.getName()); | |
| playerData.savePlayerDataAsync(uuid, data); | |
| } else { | |
| // Update login timestamp for existing players | |
| playerData.updateFieldAsync(uuid, "lastLogin", System.currentTimeMillis()); | |
| playerData.updateFieldAsync(uuid, "lastKnownName", player.getName()); | |
| } |
| playerData.updateFieldAsync(uuid, "lastKnownName", player.getName()); | ||
|
|
||
| }).exceptionally(throwable -> { | ||
| getLogger().error("Failed to load data for player: " + player.getName(), throwable); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLogger() is called but it's not defined in the MyPlayerListener class. This code will not compile.
Should either:
- Add a logger field:
private static final Logger logger = LoggerFactory.getLogger(MyPlayerListener.class);and uselogger.error(...) - Pass a logger instance to the constructor
- Remove this line if it's just example code
| "lastLogout", | ||
| System.currentTimeMillis() | ||
| ).thenRun(() -> { | ||
| getLogger().debug("Updated logout time for: " + player.getName()); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLogger() is called but it's not defined in the MyPlayerListener class. This code will not compile.
Should either:
- Add a logger field:
private static final Logger logger = LoggerFactory.getLogger(MyPlayerListener.class);and uselogger.debug(...) - Pass a logger instance to the constructor
- Remove this line if it's just example code
This pull request refactors the documentation for NetworkDataAPI to clarify its role as a shared MongoDB connection layer, not an automatic player data manager. The changes emphasize best practices for plugin developers, detail recommended usage patterns, and introduce a new implementation notes document. The overall goal is to help developers use NetworkDataAPI correctly by managing their own data and leveraging the shared connection pool for efficiency and flexibility.
Documentation clarification and structure:
API_DOCUMENTATION.md,README.md,EXAMPLE_PLUGIN_GUIDE.md: Updated throughout to clarify that NetworkDataAPI is a shared MongoDB connection layer rather than a player data manager, with explicit lists of what it is and is not. [1] [2] [3]API_DOCUMENTATION.md: Added sections and examples showing how plugins should create and manage their own databases and collections, and why automatic player data tracking is not included. [1] [2]Best practices and usage patterns:
API_DOCUMENTATION.md,IMPLEMENTATION_NOTES.md: Provided detailed examples and recommendations for action-based data creation (not on player join), use of async operations, and index creation for performance. [1] [2]Implementation notes and philosophy:
IMPLEMENTATION_NOTES.md: Added a comprehensive new document outlining the design philosophy, removed features, recommended usage patterns, and performance considerations for NetworkDataAPI.Example plugin guidance:
EXAMPLE_PLUGIN_GUIDE.md: Refined to demonstrate correct usage of NetworkDataAPI as a connection layer, including custom collection management and CRUD operations.API usage improvements:
API_DOCUMENTATION.md: Updated code samples to use the correct package names and modern best practices for database access.These changes will help developers understand how to use NetworkDataAPI efficiently and avoid common pitfalls when integrating MongoDB into their Minecraft plugins.