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

Fix error when unloading a script with multiple variables sections #6047

Merged
merged 8 commits into from Sep 22, 2023
Merged

Fix error when unloading a script with multiple variables sections #6047

merged 8 commits into from Sep 22, 2023

Conversation

Moderocky
Copy link
Member

Description

In 2.7.0 and potentially the latest 2.7 beta, having multiple variables: sections in a script would produce a severe error when trying to unload or reload.

This is described in issue #6013.

The cause of this error was both variables sections using the same DefaultVariables.class key in the data map, so the first one to unload would remove the data, leaving it null for the second unload.

As version 2.6.4 supported multiple variables: sections (although potentially unintentionally) I have re-created the original behaviour with a bandage bug fix.

Note: I make no comment about whether this behaviour is correct or incorrect, intended or unintended. This is simply a fix to restore the old, expected behaviour for the time being.

I used the following code to test:
image

Version 2.6.4 produces the following output, which is what I am aiming for:
image

Version 2.7.0 produces the following output, and errors when unloading:
image

This is because the first variables section is discarded when the second one is parsed, and so the first one will never be loaded.

My fix produces the following output:
image

My approach was to look for an existing variables section when parsing and insert its contents first (therefore giving it precedence) and replace it with the new, combined version.
This appears to produce the closest match to the 2.6.4 behaviour.

Target Minecraft Versions:
Requirements:
Related Issues: #6013

@Moderocky
Copy link
Member Author

I'm not intending this to be a permanent fix (we're not actually sure what the behaviour should be) but I want to restore the old behaviour as a bug fix for 2.7.1.

APickledWalrus
APickledWalrus requested changes Sep 20, 2023
View reviewed changes
src/main/java/ch/njol/skript/structures/StructVariables.java Outdated
return true;
}

@Override
public boolean load() {
DefaultVariables data = getParser().getCurrentScript().getData(DefaultVariables.class);
if (data == null) // band-aid fix for this section's behaviour being handled by a previous section
Copy link
Member

Choose a reason for hiding this comment

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

if data is null here, then something has gone really wrong

We should prefer an assertion or throw an error - or do nothing at all because there's no way it could be null unless something (e.g. an addon) has dangerously manipulated things (which could happen anywhere)

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know if there was a situation where one could load and unload before another loads so I thought I'd better make sure 😬

Copy link
Member

Choose a reason for hiding this comment

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

ah it shouldn't. if this did somehow happen we definitely need some sort of feedback (whether that's us checking and throwing an error or allowing the code to error)

src/main/java/ch/njol/skript/structures/StructVariables.java Show resolved Hide resolved
src/main/java/ch/njol/skript/structures/StructVariables.java Outdated
List<NonNullPair<String, Object>> variables;
Script script = getParser().getCurrentScript();
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO variables: sections
if (existing != null && existing.hasDefaultVariables())
Copy link
Member

Choose a reason for hiding this comment

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

Not easy to write a suggestion as I'm on mobile, but should have curly braces for this if-else structure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't worry I got you covered

@sovdeeth sovdeeth added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Sep 20, 2023
APickledWalrus
APickledWalrus approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

approved as the requested change is minor

src/main/java/ch/njol/skript/structures/StructVariables.java Outdated
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

star import should be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

oh my bad I didn't notice

@Moderocky Moderocky added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Sep 22, 2023
AyhamAl-Ali
AyhamAl-Ali approved these changes Sep 22, 2023
View reviewed changes
src/main/java/ch/njol/skript/structures/StructVariables.java

List<NonNullPair<String, Object>> variables = new ArrayList<>();
List<NonNullPair<String, Object>> variables;
Script script = getParser().getCurrentScript();
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a user just writes down variables: in effect commands? does it throw a NPE?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing should happen since effect commands are parsed as an effect :)

src/main/java/ch/njol/skript/structures/StructVariables.java
List<NonNullPair<String, Object>> variables = new ArrayList<>();
List<NonNullPair<String, Object>> variables;
Script script = getParser().getCurrentScript();
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO variables: sections
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO variables: sections
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO+ variables: sections

@Moderocky Moderocky merged commit bfa4421 into SkriptLang:dev/patch Sep 22, 2023
4 checks passed
Moderocky added a commit that referenced this pull request Sep 24, 2023
@Moderocky @TheLimeGlass @AyhamAl-Ali
Merge branch 'dev/patch' into dev/feature (#6065)
f348832 
* Fix ExprRandomNumber using a method from Java 17 (#6022)

* Fix changing remaining time of command cooldown (#6021)

Update ScriptCommand.java

Co-authored-by: Moderocky <admin@moderocky.com>

* Bump version to 2.7.1 (#5993)

Co-authored-by: Moderocky <admin@moderocky.com>

* fix 3 stray INSERT VERSIONs from 2.7.0 (#6027)

correct incorrect values

* Fix Documentation Actions on dev/patch (#6042)

* Tidy up parts of config class. (#6025)

* Add Release Model Document (#6041)

Add release model document

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* (Cherry Pick) Fix cast throwing if existing variable for command storage exists (#5942) (#6026)

Fix cast throwing if existing variable for command storage exists (#5942)

* Fix cast throwing if existing variable for command storage exists

* Update src/main/java/ch/njol/skript/command/ScriptCommand.java



---------

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* (Cherry Pick) Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978) (#6023)

Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978)

* Avoid NPE and clean up class

* Update ExprEntityAttribute.java

* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java



* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java

---------

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* Fix multiple aliases sections not working (#6050)

* Fix error when unloading a script with multiple variables sections (#6047)

* Returns the old 2.6.4 duplicate variables section behaviour.

* Add an error but i don't know what it's for

* Add lots of brackets to keep walrus happy :}}}

* Add load tracker to prevent multiple loading.

* Prevent variable data wipe, fix another bug

* Support IDEs from the dark ages that don't know what a star is and think it orbits the earth or something

* add a test

---------

Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>

* Bump actions/checkout from 3 to 4 (#6029)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

* ⚒ Disable Javadocs generation for nightly docs & improvements (#6059)

* Let's see if I am good at GH actions 🤞

* ops!

* Use proper docs template reference when possible

* Disable nightly javadocs generation with an option

Each javadoc is ~50mb, which was causing the big size of the docs! while each docs generation is ~2mb only

* Fix building

* Revert pull changes

They are not what fixed the issue, probably the old PRs aren't syncing for some reason

* Update build.gradle

---------

Co-authored-by: Moderocky <admin@moderocky.com>

* Change the target branch of dependabot (#6063)

Update dependabot.yml

* Update cleanup-docs.yml

---------

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
sovdeeth added a commit that referenced this pull request Dec 17, 2023
@APickledWalrus @UnderscoreTud
@sovdeeth @Moderocky @TheLimeGlass @AyhamAl-Ali @dependabot @NotSoDelayed @Spongecade @MihirKohli @3meraldK
Merge master into dev/feature (#6211)
09e8527 
* Fix ExprRandomNumber using a method from Java 17 (#6022)

* Fix changing remaining time of command cooldown (#6021)

Update ScriptCommand.java

Co-authored-by: Moderocky <admin@moderocky.com>

* Bump version to 2.7.1 (#5993)

Co-authored-by: Moderocky <admin@moderocky.com>

* fix 3 stray INSERT VERSIONs from 2.7.0 (#6027)

correct incorrect values

* Fix Documentation Actions on dev/patch (#6042)

* Tidy up parts of config class. (#6025)

* Add Release Model Document (#6041)

Add release model document

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* (Cherry Pick) Fix cast throwing if existing variable for command storage exists (#5942) (#6026)

Fix cast throwing if existing variable for command storage exists (#5942)

* Fix cast throwing if existing variable for command storage exists

* Update src/main/java/ch/njol/skript/command/ScriptCommand.java



---------

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* (Cherry Pick) Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978) (#6023)

Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978)

* Avoid NPE and clean up class

* Update ExprEntityAttribute.java

* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java



* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java

---------

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* Fix multiple aliases sections not working (#6050)

* Fix error when unloading a script with multiple variables sections (#6047)

* Returns the old 2.6.4 duplicate variables section behaviour.

* Add an error but i don't know what it's for

* Add lots of brackets to keep walrus happy :}}}

* Add load tracker to prevent multiple loading.

* Prevent variable data wipe, fix another bug

* Support IDEs from the dark ages that don't know what a star is and think it orbits the earth or something

* add a test

---------

Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>

* Bump actions/checkout from 3 to 4 (#6029)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

* ⚒ Disable Javadocs generation for nightly docs & improvements (#6059)

* Let's see if I am good at GH actions 🤞

* ops!

* Use proper docs template reference when possible

* Disable nightly javadocs generation with an option

Each javadoc is ~50mb, which was causing the big size of the docs! while each docs generation is ~2mb only

* Fix building

* Revert pull changes

They are not what fixed the issue, probably the old PRs aren't syncing for some reason

* Update build.gradle

---------

Co-authored-by: Moderocky <admin@moderocky.com>

* Change the target branch of dependabot (#6063)

Update dependabot.yml

* ⚒ Fix stop all sounds NPE (#6067)

* Bump actions/checkout from 3 to 4 (#6069)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Bump org.gradle.toolchains.foojay-resolver-convention from 0.5.0 to 0.7.0 (#6070)

Bump org.gradle.toolchains.foojay-resolver-convention

Bumps org.gradle.toolchains.foojay-resolver-convention from 0.5.0 to 0.7.0.

---
updated-dependencies:
- dependency-name: org.gradle.toolchains.foojay-resolver-convention
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Bump org.easymock:easymock from 5.1.0 to 5.2.0 (#6071)

Bumps [org.easymock:easymock](https://github.com/easymock/easymock) from 5.1.0 to 5.2.0.
- [Release notes](https://github.com/easymock/easymock/releases)
- [Changelog](https://github.com/easymock/easymock/blob/master/ReleaseNotes.md)
- [Commits](easymock/easymock@easymock-5.1.0...easymock-5.2.0)

---
updated-dependencies:
- dependency-name: org.easymock:easymock
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Bump io.papermc.paper:paper-api from 1.20.1-R0.1-SNAPSHOT to 1.20.2-R0.1-SNAPSHOT (#6072)

* Bump io.papermc.paper:paper-api

Bumps io.papermc.paper:paper-api from 1.20.1-R0.1-SNAPSHOT to 1.20.2-R0.1-SNAPSHOT.

---
updated-dependencies:
- dependency-name: io.papermc.paper:paper-api
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Apply 1.20.2 to the test runner

* Deprecate org.bukkit.util.Consumer usage in EntityData

* Adapt against the Java Consumer instead of Bukkit's

* Resolve existing method deprecation

* Adapt against the Java Consumer instead of Bukkit's

* Update developer note

* Result in reflection for Bukkit Consumer

* Resolve ThrownPotion Consumer

* Result in reflection for Bukkit Consumer

* Pretty else if

* Add common reflective spawn method.

* Use common spawn method in potion class.

* Remove old suppression!

* Whoops I forgot about the consumer

* Don't need reflection import anymore :)

* Thrown potion class

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: TheLimeGlass <seantgrover@gmail.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* Pull request template defaults (#5665)

Update pull_request_template.md

* Fix EvtPlayerChunkEnter Comparison & Cleanup (#5965)

Initial

(cherry picked from commit 389c002)

Co-authored-by: Moderocky <admin@moderocky.com>

* Fixes EffSecSpawn not properly handling local variables created within the section (#6033)

Communicate local variables between consumer calls

thanks pickle

Co-authored-by: Moderocky <admin@moderocky.com>

* Remove PlayerPreprocessCommandEvent listener and clean up Commands (#5966)

* Remove PPCE listener and clean up Commands

* Apply suggestions from code review

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Update Commands.java

* we hate breaking changes

---------

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* Clean up vector classes and fix a few bugs.

* More improvements

* Apply suggestions from code review

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* Budget Expansion

* Fix Logging Issues In ExpressionEntryData (#6081)

Fix duplicate logging

* Prepare For Release 2.7.1 (#6082)

* Update Minecraft wiki links to new domain (#6078)

* ⚒ Fix fake player count paper check error (#6090)

* Fix Command Help (#6080)

Fix issues and cleanup CommandHelp class

Co-authored-by: Moderocky <admin@moderocky.com>

* Bump net.kyori:adventure-text-serializer-bungeecord from 4.3.0 to 4.3.1 (#6084)

Bumps [net.kyori:adventure-text-serializer-bungeecord](https://github.com/KyoriPowered/adventure-platform) from 4.3.0 to 4.3.1.
- [Release notes](https://github.com/KyoriPowered/adventure-platform/releases)
- [Commits](KyoriPowered/adventure-platform@v4.3.0...v4.3.1)

---
updated-dependencies:
- dependency-name: net.kyori:adventure-text-serializer-bungeecord
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

* Fix unloading/reloading a directory in the scripts effect (#6106)

* Moved unloading to a common method.

* Accidental double space 😬

* Force UTF-8 encoding for Gradle daemon (#6103)

Co-authored-by: Moderocky <admin@moderocky.com>

* Corrected Javadocs name, title (#6038)

* Javadoc Title,Name

* Update .gitignore

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

---------

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* Rebase JUnit references fix for dev/patch (#6057)

* Fix options issue in functions (#6121)

* Fix command permission messages (2.7.1 issue) (#6126)

re-add permission handling during PPCE to get around spigot behavior.

* Fix stack overflow when stringifying block inventories. (#6117)

* Fix stack overflow when stringifying block inventories.

* Blanket catch until a better solution is found.

* Fix comparison of cyclical types (specifically comparing times) (#6128)

* Add cyclical type helper.

* Make time cyclical.

* Add special comparison for cyclical types.

* Add some tests.

* Fix floating point rounding error in loop N times (#6132)

Fix floating point error in loop X times.

* Fix Sorted List Expression (#6102)

* Fix ExprSortedList

* Update src/main/java/ch/njol/skript/expressions/ExprSortedList.java

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Fix test

Co-authored-by: Moderocky <admin@moderocky.com>

---------

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

* Fix colour codes being reset in reload message. (#6150)

Fix colour codes being reset.

* Fix ExprDurability's Changer (#6154)

* Fix ExprDurability's changer

* Change method name.

* Add simple test.

---------

Co-authored-by: Moderocky <admin@moderocky.com>

* Catch the exception when pushing entity by non finite vector (#5765)

* Fix issues with ExprDrops (#6130)

* refactor ExprDrops, fix bugs, add test

fixed setting drops to multiple items/experience values at once
fixed null values being left in drops after removing items
maintained behavior but behavior needs a big update

* small cleanup

* Fix JUnit test location

* import shenanigans

---------

Co-authored-by: Moderocky <admin@moderocky.com>

* Prepare For Release (2.7.2) (#6166)

* Prevent InventoryHolder -> X chaining (#6171)

* Prevent InventoryHolder -> X chaining

* Improve Location Comparison (#6205)

* Allow asynchronous SkriptEvent#check execution (#6201)

* Fix ExprSets conflicting (#6123)

* Prepare for Release (2.7.3) (#6208)

* Further corrections

* Fix NPE issue with drops in 1.20.2

* Update StructFunction.java

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: _tud <mmbakkar06@gmail.com>
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: TheLimeGlass <seantgrover@gmail.com>
Co-authored-by: DelayedGaming <72163224+DelayedGaming@users.noreply.github.com>
Co-authored-by: Spongecade <spongecade.129@gmail.com>
Co-authored-by: MihirKohli <55236890+MihirKohli@users.noreply.github.com>
Co-authored-by: _tud <98935832+UnderscoreTud@users.noreply.github.com>
Co-authored-by: 3meraldK <48335651+3meraldK@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus approved these changes

@AyhamAl-Ali AyhamAl-Ali AyhamAl-Ali approved these changes

Assignees
No one assigned
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Moderocky @AyhamAl-Ali @APickledWalrus @sovdeeth