panic: assignment to entry in nil map

[michielbdejong]

This error messages seems to be happening on master since last week.

[michielbdejong]

Work-around: git checkout 392b4 and make build-revad

[michielbdejong]

Confirmed if you git checkout a2b2109 you get the panic.
Very weird that CI didn't pick this up when #3401 was reviewed!

[michielbdejong]

@glpatcern can you confirm?

[glpatcern]

Indeed weird that it was not picked up in the CI (and it didn't even fire doing tests). I'll have a look.

[glpatcern]

OK, issue understood and fixed, PR on the way

[michielbdejong]

You mean both the bug and the reason that CI didn't catch it?

[glpatcern]

The bug was due to a bad assumption in case of invalid config, about the CI I guess it does not test such invalid configs.

[michielbdejong]

Hm, I think we have two options:

• call an empty mimetypes config "invalid", in which case this should be documented on reva.link, and also the error message should be 'empty mimetypes config not allowed' instead of panic. This would require:
  • change the error message
  • add a test
  • add docs
  • change all production configs
• call an empy mimetypes config "valid", in which case no documentation change is needed, and no config change in production systems; we just need:
  • the bugfix you already wrote glpatcern@57d686c
  • add a test where mimetype config is empty. This test would fail in a2b2109, but pass with your patch

I think the second one is less work?

[glpatcern]

Hi @michielbdejong, sorry I was too fast on Saturday: in fact a configuration with no parameters is perfectly valid for a driver such as the demo appprovider which, well, does not need parameters as it does not do much anyway.

So apart from doing "more or less work", the empty driver config is totally legitimate (and BTW that was the issue, not the empty mimetypes, which are fully optional on their own).

Now concerning a validation test, here we'd need to test for a demo driver with empty config. I can look into that.

[michielbdejong]

You can try loading this config in a test, I think you will be able to reproduce the panic with that config file and with your patch not applied.

[michielbdejong]

This issue was closed when the PR was merged, but we still need to write the test for it.

@glpatcern will you still write that test, or should I?

[glpatcern]

Yes, that was the intention but today I got stuck with other things (and I anticipate tomorrow will be the same...), will push a PR with the test on Wednesday.

[glpatcern]

@michielbdejong the draft PR was created in #3459, but it turns out I can't make the test fail: the reason is that the tests are very superficial and only validate the parsing, not the real initialization process - which is the one that led to the (fixed) panic. So as things stand, it would require quite some mocking (including a mocked gateway service) to be able to test more than just the config parsing.

With that, my proposal is to close that PR (yet the changes in the docs are to be merged eventually) and even drop a misleading test for the wopi driver, because it is not even touching the wopi.go code, and instead go for integration tests in the CI (there's something being cooked up), where revad is started with a valid config out of the examples and clearly should not panic. What do you think?

[michielbdejong]

Is there a GitHub issue for the integration tests in the CI?
I'll close this now, thanks for all the digging you did!

[glpatcern]

It was a good exercise nevertheless! The GitHub issue is not yet there, we have an internal one (@gmgigi96 is working on it), but it will come soon.