AuthMe / ConfigMe

A simple configuration management library for any Java project!
MIT License
37 stars 16 forks source link

ConfigurationData#setValue() doesn't offer a way to set value of MapProperty #412

Closed drori200 closed 8 months ago

drori200 commented 8 months ago

I created a MapProperty doing the following in my config:

private static Map<String, Integer> createDefaultMap() {
        Map<String, Integer> map = new HashMap<>();
        map.put(Ref.STR.getValue(), 10);
        map.put(Ref.DEX.getValue(), 9);
        map.put(Ref.CON.getValue(), 8);
        map.put(Ref.WIL.getValue(), 7);
        map.put(Ref.MND.getValue(), 6);
        map.put(Ref.SPI.getValue(), 5);

        // Organizes the Map properly
        Map<String, Integer> treeMap = new TreeMap<String, Integer>(map);

        return treeMap;
    }
    @Comment("The amount of stat points the player will get for each stat type.")
    public static final MapProperty<Integer> STAT_AMOUNT = new MapProperty<>("StatAmount", createDefaultMap(),
            PrimitivePropertyType.INTEGER);

Now i am in the process of migrating my old config system but when i try to use the configurationData parameter from MigrationService#performMigrations there is nothing offered to set the value of the MapProperty STAT_AMOUNT

drori200 commented 8 months ago

The same goes for the reader property not providing anything for getting a Map

ljacqu commented 8 months ago

ConfigurationData#setValue allows to pass in a value that matches the property type, so passing a map for your StatAmount will definitely work. PropertyReader reflects the YAML structure and so is more or less untyped in its return values, save for a few convenience methods I'm not too fond of.

If I understand your setup correctly, I think you have two ways to approach this. Either enumerate the old paths, retrieve them with a Property implementation and copy it over to the new path, or check generically if there is a map in the old place and take it from there. The first approach is easier to understand and less error-prone, I guess, but doesn't scale well. I'd recommend the second approach if you have a number of paths that is too much to list out.

First approach—get old paths with a local Property, if there is something, take it over:

    public class StatMigrationService implements MigrationService {

        /*
         * Example for a YAML like:
         * oldStats:
         *   oldStr: 3
         *   oldWil: 8
         *
         * Where oldStr and oldWil should be taken over into Ref.STR and Ref.WIL entries in the new map property.
         */
        @Override
        public boolean checkAndMigrate(@NotNull PropertyReader reader, @NotNull ConfigurationData configurationData) {
            if (!reader.contains("oldStats")) {
                return false; // Old path does not exist; nothing to migrate
            }

            Map<String, Integer> statValues = getCurrentStatValues(reader);
            // Note: Chain statements with | and not || to make sure all are performed
            boolean hasOldProperty =
                setOldStatsValueIfPresent(statValues, reader, "oldStats.oldStr", Ref.STR)
                | setOldStatsValueIfPresent(statValues, reader, "oldStats.oldDex", Ref.DEX)
                | setOldStatsValueIfPresent(statValues, reader, "oldStats.oldCon", Ref.CON)
                | setOldStatsValueIfPresent(statValues, reader, "oldStats.oldWil", Ref.WIL)
                | setOldStatsValueIfPresent(statValues, reader, "oldStats.oldMnd", Ref.MND)
                | setOldStatsValueIfPresent(statValues, reader, "oldStats.oldSpi", Ref.SPI);

            if (hasOldProperty) {
                configurationData.setValue(StatsSettingsHolder.STAT_AMOUNT, statValues);
            }
            return hasOldProperty;
        }

        // Gets the current stats under the new path, ensuring that the default value is not used
        // if no values are present yet
        private Map<String, Integer> getCurrentStatValues(PropertyReader reader) {
            PropertyValue<Map<String, Integer>> propertyValue =
                StatsSettingsHolder.STAT_AMOUNT.determineValue(reader);
            if (propertyValue.isValidInResource()) {
                return propertyValue.getValue();
            }
            return new TreeMap<>();
        }

        private boolean setOldStatsValueIfPresent(Map<String, Integer> statValues,
                                                  PropertyReader reader,
                                                  String oldPath, Ref newStatsReference) {
            Property<Integer> oldProp = PropertyInitializer.newProperty(oldPath, -1);
            PropertyValue<Integer> value = oldProp.determineValue(reader);
            if (value.isValidInResource()) {
                // #putIfAbsent to not have the old path override the new path's value since
                // statsValues only has values that are actually in the YAML
                statValues.putIfAbsent(newStatsReference.getValue(), value.getValue());
                return true;
            }
            return false;
        }
    }

Skeleton of the second approach:

    public static class GenericMapMigrationService implements MigrationService {

        /*
         * Example for a YAML like:
         * oldStats:
         *   oldStr: 3
         *   oldWil: 8
         *
         * Where oldStr and oldWil should be taken over into Ref.STR and Ref.WIL entries in the new map property.
         */
        @Override
        public boolean checkAndMigrate(@NotNull PropertyReader reader, @NotNull ConfigurationData configurationData) {
            Object oldStatsValue = reader.getObject("oldStats");
            if (oldStatsValue instanceof Map<?, ?>) {
                // getCurrentStatValues same impl. as in code block above
                performOldStatsMigration((Map<?, ?>) oldStatsValue,
                    getCurrentStatValues(reader), configurationData);
                return true;
            }
            return false;
        }

        private void performOldStatsMigration(Map<?, ?> oldStatsValue,
                                              Map<String, Integer> newStatValues,
                                              ConfigurationData configurationData) {
            boolean hasChange = false;
            for (Map.Entry<?, ?> entry : oldStatsValue.entrySet()) {
                Object key = entry.getKey();
                Object value = entry.getValue();
                if (key instanceof String && value instanceof Integer) {
                    // Determine new path based on key, set value to newStatValues
                    // newStatValues.putIfAbsent(newPath, (Integer) value);
                    hasChange = true;
                }
            }
            if (hasChange) {
                configurationData.setValue(StatsSettingsHolder.STAT_AMOUNT, newStatValues);
            }
        }
    }

The migration is a little more difficult than you might expect at first glance. I don't have a clear picture of how you're using this property value, but if you're absolutely expecting that every Ref entry be present in the map, I'd create a dedicated property for each stat instead of putting them all in a map. That way, you can address them individually in migrations (using moveProperty on PlainMigrationService) and retrievals and you're sure that all values are present. With the map property, you're never guaranteed that every Ref will be in it, e.g. if a user deletes on of the entries in the YAML. You'd have to separately guard against this.

drori200 commented 8 months ago

The Ref class is just an Enum that holds some values, in this case Ref.STR and such is just a string with the name of the stat.

In my current case it is related to #411 where the same situation of migrating from an old config system.

To get the values for the old config file i did the following:

File oldConfigFile = new File(Ref.puDir, "PUItem.yml");
YamlFileReader oldConfig = new YamlFileReader(oldConfigFile.toPath());

To use setValue using StatsSettingsHolder.STAT_AMOUNT for it to have the old config value i'd have to transform oldConfig.getObject("PUItem(itemID).statAmounts") to a map (since there is no way to get a map from the file So the method would look like this: configurationData.setValue(StatsSettingsHolder.STAT_AMOUNT, (transformed to map) oldConfig.getObject("PUItem(itemID).statAmounts"))

drori200 commented 8 months ago

After a short search I found this article for using GSON to turn an Object into map https://www.baeldung.com/java-convert-object-to-map#using-gson I decided to use this and it works great but I rather not depend on another dependency

This is my usage:

File oldConfigFile = new File(Ref.puDir, "PUItem.yml");
            YamlFileReader oldConfig = new YamlFileReader(oldConfigFile.toPath());
            String sectionName = "PUItem" + itemID;
            if(oldConfig.contains(sectionName)) {
                Gson gson = new Gson();
                String json = gson.toJson(oldConfig.getObject(sectionName + ".statAmounts"));
                Map<String, Integer> map = gson.fromJson(json, new TypeToken<Map<String, Integer>>() {}.getType());
                System.out.print("stats " + map);
configurationData.setValue(PUConfig.STAT_AMOUNT, map); 
}
ljacqu commented 8 months ago

If your old data is in a separate file, you can use the approaches I suggested in the previous comment by creating your own YamlFileReader as you've shown in your own code:

YamlFileReader oldConfig = new YamlFileReader(oldConfigFile.toPath());

YamlFileReader#getObject returns whatever is under the given path, so it's a question of checking whether there is a map there (second approach) or get each old path individually and try to convert it to an integer (first approach)