No error when 2 materials with the same names about gocd-yaml-config-plugin HOT 18 CLOSED

the YAML file is read using com.esotericsoftware.yamlbeans.YamlReader in this plugin... it automatically will override the value for a keyword with the last value found in the yaml. so e.g last material definition of 'upstream' in this issue. So, it'll load following:

      upstream:
        pipeline: P2
        stage: itest

@tomzo I am thinking to fix this by introducing a YAML validator, but wanted to get your opinion as well on the approach. How do you think we can best handle this?

from gocd-yaml-config-plugin.

As you have noted, the problem is that after yamlbeans has loaded yaml into a map here there is no way to tell that there was a duplicate key in yaml file. com.esotericsoftware.yamlbeans.YamlReader looses that information.
I think the correct way to fix this is by overriding this method in YamlReader or even copy entire YamlReader class to source here and modify as needed. Essentially we want to ensure that it does not override the value for a keyword with the last value found in the yaml. Such fix would take care of a few other places where similar errors can be make: pipelines:, jobs:, :tabs, etc.- all these require unique keys.
That was my idea so far, but I think YAML validator is a good approach too, I guess it has more applications then, e.g. to validate keywords.

from gocd-yaml-config-plugin.

how about using the yamllint as the first step here? yamllint gives error on finding duplicate keys in a yaml and we can throw that error out. we will have add yamllint as a gradle dependency. What I have not yet been able to figure out yet though, is how to use call its validator from the java program :(

from gocd-yaml-config-plugin.

I don't think that the maven plugin you pointed actually uses the python yamllint. It is actually based on snakeyaml, see this dependency. I don't think it is worth going outside of java just to implement validation.
If snakeyaml fails on duplicate keys (I'm not sure if it does), we can also change parser to snakeyaml, but that is quite some work.

from gocd-yaml-config-plugin.

I raised a similiar question on the yamlbeans google fourm. does this makes any sense to you @tomzo ? I think we are close, but we'l have to override YAMLReader methods!

from gocd-yaml-config-plugin.

It seems right. Although the method mentioned by Nate is private so I guess it has to be duplicated here.

I think we can add a class ValidatingYamlReader inheriting YamlReader with readValue overriden

@Override
protected Object readValue (Class type, Class elementType, Class defaultType) throws YamlException, ParserException,
        TokenizerException {

// same as in original
//...
        return readValueInternalNoDuplicates(this.chooseType(tag,  defaultType, type), elementType, anchor);
    }

And then copy implementation of readValueInternal to a new method readValueInternalNoDuplicates and implement unique key checking.

Another option would be to contribute to yamlbeans and add there an option for validating duplicate keys.

from gocd-yaml-config-plugin.

yea, I asked Nate if he thinks that its a right feature to be added to the YamlBeans itself, then I can contribute there...

agree that we need a readValueInternalNoDuplicates method with following implementation:

after reading the code, I think we'll have to declare a set of keys just before the while loop beings here and will have add a check just before this line here, where we know what key is being added to the current HashMap object. what do you think @tomzo ?

from gocd-yaml-config-plugin.

It sounds about right.
I would start by adding a test which reproduces duplicate-key case, debug the code to see where the value is replaced, then I would attempt at making changes in readValueInternal.

from gocd-yaml-config-plugin.

👍 absolutely. let me take a stab at that!

from gocd-yaml-config-plugin.

Great, thanks for signing up on this :)

from gocd-yaml-config-plugin.

@tomzo because of the difficulties as explained here in extending the YamlReader class as we decided above, what I ended up doing is raising a pull request to yamlbeans. I hope this gets accepted. Once that is done, we can rather change the usage of YamlReader here to something like below:

YamlReader reader = new YamlReader(new InputStreamReader(inputStream), false);
// the second parameter is the addition which I did which is boolean flag to set allowDuplicates. in our case it'll be false and we'll get YamlException which we can catch and throw back appropriately and we should be good

I'll add appropriate tests to support the same. Let me know if you see any concerns or think differently.

from gocd-yaml-config-plugin.

I skimmed through the PR. Looks very good.
Calling yaml reader with just one option seems like much cleaner solution than copy-pasting methods here. 👍

from gocd-yaml-config-plugin.

@tomzo kind of got stuck here at the last step... my PR shows that the whole files changed because of line endings etc.. I tried correcting it, but not sure where I am going wrong... do you have any pointers how to fix it!!

from gocd-yaml-config-plugin.

@sanjusoftware try some of these to add carriage returns.

from gocd-yaml-config-plugin.

@tomzo I am not able to move forward.. really stuck..not able to convert the files in PR back to how they should have been!! would be great if you could help :(

from gocd-yaml-config-plugin.

@tomzo finally, our change to the yamlbeans is here. waiting for @NathanSweet to release a new version of the jar :) 👍

from gocd-yaml-config-plugin.

should be good to close this one now! 👍

from gocd-yaml-config-plugin.

Fixed by #17, released in 0.3.0

from gocd-yaml-config-plugin.