[BUG] @Nullable on com.ldtteam.structurize.util.BlueprintPositionInfo#getBlockInfo not handled in various places in code
Akjosch opened this issue · 13 comments
Is there an existing issue for this?
- I have searched the existing issues
Are you using the latest MineColonies Version?
- I am running the latest alpha version of MineColonies for my Minecraft version.
Did you check on the Wiki? or ask on Discord?
- I checked the MineColonies Wiki and made sure my issue is not covered there. Or I was sent from discord to open an issue here.
What were you playing at the time? Were you able to reproduce it in both settings?
- Single Player
- Multi Player
Minecraft Version
1.20.1
MineColonies Version
1.20.1-1.1.483, version/main code branch
Structurize Version
1.20.1-1.0.697, 1.20.1-1.0.699 and 1.20.1-1.0.700
Related Mods and their Versions
Shouldn't matter
Current Behavior
At currently 21 places (in 20 lines) in code, Minecolonies uses info.getBlockInfo().getState(), with info being a com.ldtteam.structurize.util.BlueprintPositionInfo instance. The problem is that com.ldtteam.structurize.util.BlueprintPositionInfo#getBlockInfo can return null (and is correctly annotated with @Nullable), and so all those places are a potential NPE waiting to happen.
And in fact, it does happen for me, with the result in log looking typically something like this:
[21:09:33] [Server thread/WARN]: Statemachine for state BUILDING_STEP threw an exception:
java.lang.NullPointerException: Cannot invoke "com.ldtteam.structurize.util.BlockInfo.getState()" because the return value of "com.ldtteam.structurize.util.BlueprintPositionInfo.getBlockInfo()" is null
at com.minecolonies.core.entity.ai.workers.AbstractEntityAIStructure.lambda$structureStep$6(AbstractEntityAIStructure.java:411) ~[minecolonies-1.20.1-1.1.483-BETA.jar%23268!/:1.20.1-1.1.483-BETA]
(... rest skipped, not relevant here...)
Relevant code points:
- (twice)
Expected Behavior
Check for null return values from @Nullable methods.
Reproduction Steps
Have a builder try to upgrade a building where they have to replace an air block with something else at some point (which affects almost all up gradable structures). Potentially start the upgrade before the commit in #9735 (which added a lot of those places), update the mod, then load the world again; but I didn't test that possibility yet.
Logs
Not relevant
Anything else?
- Add a thumbs-up to the bug report if you are also affected. This helps the bug report become more visible to the team and doesn't clutter the comments.
- Add a comment if you have any insights or background information that isn't already part of the conversation.
In addition, com.ldtteam.structurize.util.BlockInfo#getState is also @Nullable, as seen here:
This should also be taken into account on a bunch of the lines above.
It should never return null at all. The reason it is causing exceptions is that somewhere it is exceeding a bound.
Well, it is and, more to the point, can return null according to Structurize's internal documentation. Is the issue then on Structurize's end, where the @Nullable annotation should be removed and the values guarded with Object.requireNonNull(...) on construction time? Because I can open an issue with the details there if required.
In a few places we might be fine with it returning null. Where we assume it does not return null it should not return null. Indeed it will only return null if we try to query something outside of the blueprint.
Like Ray said it shouldn't be null in the first place. Javadoc is sometimes dated.
Simply putting an if statement with a non null check doesn't solve the underlying issue, if you do that they will skip stuff instead of crash.
You rather want your building full of holes and an unexplainable issue?
Javadoc is sometimes dated.
That's why I asked if I should open a Structurize issue instead. As a general rule, I prefer to not suggest API changes if I can help it, but you know your code better than I do obviously.
In a few places we might be fine with it returning null.
There's not a single piece of code currently inside MineColonies where you call that method and not call getState() on the result immediately, without checking for null. So those "few places" are exactly "zero places".
@someaddons The issue of Minecolonies not using the Structurize API properly is in the code, not the logs. The logs just show a symptom of it, and even then the stack trace isn't showing where the BlueprintPositionInfo with a null info field gets created, only where it gets accessed improperly.
I could recreated the stack trace (which has nothing to do with what's logged!) for it easily again if it's any help, but it's essentially https://github.com/ldtteam/Structurize/blob/2c33200215591bfafe1f79d144a0d0df0aee4bc0/src/main/java/com/ldtteam/structurize/blueprints/v1/Blueprint.java#L896 getting called with a pos argument (BlockPos[x=1,y=-1,z=0] in my case) which isn't a key in what getBlockInfoAsMap() returns. This in turn gets called in https://github.com/ldtteam/Structurize/blob/2c33200215591bfafe1f79d144a0d0df0aee4bc0/src/main/java/com/ldtteam/structurize/placement/AbstractBlueprintIterator.java#L116 - already with a non-existing positional parameter.
The NPE happens a few lines later; see attached stack trace. But that isn't the point of the issue. The point is that Structurize can and does return BlueprintPositionInfo instances with a null info field, and Minecolonies doesn't handle those cases gracefully.
I think the "just let it fail" approach only really works when you fail at the earliest possible moment, not some time later like in this case.
Be it as it may, reverting Minecolonies to 474 and Structurize to 693, then firing and re-hiring the offending builders was at least a workaround for me here. They went right back to decorating the huts they were working on before. So all I have is to find out what the other problem the builder is having with upgrading a building. But that seems unrelated (no error log, just an endless loop), so is a topic for another issue, once I gathered enough information.
As far as I know a reproduce is shutting down the world during a builder water clearing step. This messes up the progress pos state somehow outside of boundaries is my guess.
As far as I know a reproduce is shutting down the world during a builder water clearing step. This messes up the progress pos state somehow outside of boundaries is my guess.
Just in case, apparently the AI issue can still happen, so either this issue or the original issue should probably be reopened:
#9793 (comment)