parchmentmc / feather Goto Github PK

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.

An adapter to convert from/to Fabric's Mapping-IO library would be nice. I'd be willing to code this myself (most of the recent developments on the lib were contributed by me), but I first need to know if that would be accepted.

Edit: It should be noted that we're currently in the process of moving Enigma itself to said library, so this feature would allow for more direct interop without homegrown readers/writers in Compass.

Currently, MappingDataContainer.ParameterData#getJavadoc returns a single String. This precludes the possibility of multi-line javadocs for parameters, which is certainly possible both by javadoc and Enigma, the current mapping interface tool used by Parchment.

I plan to change that method to return a List<String> instead, to support the possibility of multi-line javadocs for parameters officially. This has the following consequences:

• The change in the API interface is a breaking API change and will lead to a bump of Feather's version from 1.0.1 to 2.0.0.

• The JSON format of the Mapping Data Container specification will be updated to update the javadoc key for parameters from a string to an array of strings. This necessitates a bump in the format version from the current 1.0.0 to 2.0.0.

• The parsers and writers for the MDC format (in the io-gson and io-moshi subprojects) will be updated to support 2.0.0 of the MDC JSON format, consistent with the change in interface and Feather version.

  • Support will be added to allow reading the v1.0.0 MDC format. This will likely be a default enabled option in the current parsers.

• Developers which use Feather will be informed of the breaking change sometime before Parchment exports are produced with the new format, giving them a period to update their applications to use the new Feather version and test compatibility. Consumers of their applications also need to be informed to update.

I will also look over the codebase for any API-breaking changes which can get rolled into the 2.0.0 version update.

We currently provide IO adapters for google/gson and square/moshi, two popular JSON libraries for the JVM.

For more options for consumers, we should also look into providing an IO adapter for FasterXML/jackson. Specifically, we would provide a "datatype module" -- a Module which can be registered to ObjectMapper.registerModule -- so Jackson's databind package can read and write our objects.

They'll be hooked up into the standard battery of IO tests we have already, to ensure there are no regressions or mismatched behaviors.

Our current battery of tests check that the various IO modules serialize and deserialize the data without any unwanted change or errors.

However, for wider coverage, we should also add tests for checking that invalid inputs -- missing required fields, out-of-range indexes -- are caught on deserialization.

I'm not sure how those same tests would look for serialization, if they're needed at all for it (as ideally, our data-holdings objects should be validating their arguments are as expected).

To aid in debugging, the IO adapters should incorporate in some fashion some contextual information for the object they're reading. Packages and classes should have the name, methods and fields should have the name and descriptor, and parameters should have the index.

If possible, the information should also contain the information about its parent -- methods and fields include info about their owning class, and parameters include info about their owning method.

This will probably take the form of try...catch blocks around our reading and writing methods in each adapter.