Only throw on duplicate styles in strict mode

[springmeyer]

#3770 added a throw on duplicate styles. I hit this in a system which I need back compatibility (can't easily avoid duplicate styles names). So, I think this check should only happen in strict mode.

/cc @talaj - sound good? One question I have is that I would assume this change would make the tests fail that you added in #3770 (unless the tests are put in strict mode). Any idea why they don't fail with this change as is?

[codecov]

Codecov Report

Merging #3917 into master will increase coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #3917      +/-   ##
==========================================
+ Coverage   68.78%   68.79%   +<.01%     
==========================================
  Files         442      442              
  Lines       23258    23266       +8     
==========================================
+ Hits        15998    16005       +7     
- Misses       7260     7261       +1

Impacted Files	Coverage Δ
src/load_map.cpp	87.09% <90%> (+0.11%)	⬆️
include/mapnik/geometry/polygon.hpp	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9ebc93...d60be13. Read the comment docs.

[lightmare]

You avoid the config_error("duplicate style name") but still throw config_error("failed to insert style to the map"). The test requires a throw, doesn't check the message.

[springmeyer]

Nice catch, thank you @lightmare. Tried to fix it to not throw (now) unless strict mode is on: 434511c. But that appears to still pass, so clearly I'm missing something.

[lightmare]

$ test/standalone/map_xml_test-bin -s | grep -A4 duplicate
  path.native() := test/data/good_maps/duplicate_stylename.xml

test/standalone/map_xml_test.cpp:152: 
PASSED:
  REQUIRE_NOTHROW( load_map(m, path) )
--
  path.native() := test/data/broken_maps/duplicate_stylename.xml

test/standalone/map_xml_test.cpp:169: 
PASSED:
  REQUIRE_THROWS( mapnik::load_map(m, path.native(), true) )

They pass because the broken_maps test explicitly sets strict=true.

[springmeyer]

With the last test fix I think this is ready to merge. Want to give a PR approval @lightmare?

[lightmare]

:normal dk
(that's vim command to delete this line and the one above ;))

[springmeyer]

nice catch! thank you. fixed in d60be13

[lightmare]

An aside comment. I haven't yet looked into what are the differences between strict/non-strict, whether it's supposed to persist or is just some temporary back-compatibility measure. But if it's something that affects many different parts of parsing and is long-lived, it might be worth having separate broken_maps_strict that are broken under strict=true but good under strict=false.

[springmeyer]

it might be worth having separate broken_maps_strict that are broken under strict=true but good under strict=false.

That seems like a good idea. I think, if I remember right, that strict/non-strict mostly exists because of how fonts work in the renderer - fallbacks work still if some fonts are not able to be registered at map load. So non strict mode allows rendering to gracefully degrade. And since then we've hooked into it as a back compatibility measure, so all devs to opt-into new stricter checks when they have total . control over their system (like at map creation) and opt-out when they don't have control (existing working maps that should stay working).