ParchmentMC / Feather

Library for common data objects used by ParchmentMC projects.
MIT License
4 stars 6 forks source link

Parameter indexes cannot be stored safely as `byte`s #10

Open sciwhiz12 opened 10 months ago

sciwhiz12 commented 10 months ago

A glaringly large oversight I made when designing Feather was to assume that the parameter index (which is the , which can range from 0 to 255, fits in a byte. What I missed was that is true for unsigned bytes; Java's byte is a signed type, which ranges from -128 to 127. This causes issues if a parameter index actually falls after 127, which would cause a wraparound of the value (when cast to a byte) to a negative value.

While the various APIs don't seem to validate parameter indexes are positive (which is an oversight), the IO adapters do verify that; for example, the GSON adapter does that validate on read. This causes an asymmetrical problem where creating in-memory objects and exporting them is fine, but reading those exports will cause an error if they contain parameter indexes greater than 127.

The solution is to modify the API such that parameter indexes are stored in a larger data type than a byte. Using a short is acceptable, and would align with the intent of using byte to limit values to within the acceptable range of 0 to 255 for parameter indexes. However, I think usability can be improved by using an int instead, to avoid consumers needing to explicitly downcast parameter stored as ints to bytes. (We will incorporate parameter validation in the APIs, to ensure parameters are still within the 0-255 limit of the JVM.)

This will be a breaking API change, and as such slated for 2.0.0.