normalizeForVisualization() and other methods applying on a ProjectedCRS: do not mess the derivingConversion object of the original object (fixes #1736)

normalizeForVisualization(), promoteTo3D(), demoteTo2D(), alterGeodeticCRS(),
alterCSLinearUnit() and alterParametersLinearUnit() all used the object
returned by derivingConversionRef() to create a new ProjectedCRS. While doing
so, this caused the derivingConversion of the original object to have its
targetCRS set to the object returned by normalizeForVisualization() and similar.
If that object died, then the weak pointer would be reset, and the original
ProjectedCRS() has now its derivingConversionRef()->targetCRS() nullptr

So bottom line is use derivingConversion() for anything that is not pure
reading !!!

This is confirmed to be the fix for the QGIS scenario in
https://github.com/qgis/QGIS/issues/30569#issuecomment-540060919

In QGIS use case, the issue arised when using a projected CRS with a non-GIS
friendly axis (that is where normalizeForVisualization() created a new projectedCRS)

3 files changed, 79 insertions, 10 deletions

diff --git a/include/proj/crs.hpp b/include/proj/crs.hpp
index 18ac4d5d..a39d8571 100644
--- a/include/proj/crs.hpp
+++ b/include/proj/crs.hpp
@@ -507,6 +507,9 @@ class PROJ_GCC_DLL DerivedCRS : virtual public SingleCRS {
 
     PROJ_PRIVATE :
         //! @cond Doxygen_Suppress
+
+        // Use this method with extreme care ! It should never be used
+        // to recreate a new Derived/ProjectedCRS !
         PROJ_INTERNAL const operation::ConversionNNPtr &
         derivingConversionRef() PROJ_PURE_DECL;
 
diff --git a/src/iso19111/crs.cpp b/src/iso19111/crs.cpp
index 46cde43a..e044c0c7 100644
--- a/src/iso19111/crs.cpp
+++ b/src/iso19111/crs.cpp
@@ -237,7 +237,7 @@ CRSNNPtr CRS::alterGeodeticCRS(const GeodeticCRSNNPtr &newGeodCRS) const {
     auto projCRS = dynamic_cast<const ProjectedCRS *>(this);
     if (projCRS) {
         return ProjectedCRS::create(createPropertyMap(this), newGeodCRS,
-                                    projCRS->derivingConversionRef(),
+                                    projCRS->derivingConversion(),
                                     projCRS->coordinateSystem());
     }
 
@@ -264,7 +264,7 @@ CRSNNPtr CRS::alterCSLinearUnit(const common::UnitOfMeasure &unit) const {
         if (projCRS) {
             return ProjectedCRS::create(
                 createPropertyMap(this), projCRS->baseCRS(),
-                projCRS->derivingConversionRef(),
+                projCRS->derivingConversion(),
                 projCRS->coordinateSystem()->alterUnit(unit));
         }
     }
@@ -675,9 +675,8 @@ CRSNNPtr CRS::normalizeForVisualization() const {
                                               axisList[0])
                     : cs::CartesianCS::create(util::PropertyMap(), axisList[1],
                                               axisList[0], axisList[2]);
-            return util::nn_static_pointer_cast<CRS>(
-                ProjectedCRS::create(props, projCRS->baseCRS(),
-                                     projCRS->derivingConversionRef(), cs));
+            return util::nn_static_pointer_cast<CRS>(ProjectedCRS::create(
+                props, projCRS->baseCRS(), projCRS->derivingConversion(), cs));
         }
     }
 
@@ -841,7 +840,7 @@ CRSNNPtr CRS::promoteTo3D(const std::string &newName,
                                         !newName.empty() ? newName : nameStr()),
                 NN_NO_CHECK(
                     util::nn_dynamic_pointer_cast<GeodeticCRS>(base3DCRS)),
-                projCRS->derivingConversionRef(), cs));
+                projCRS->derivingConversion(), cs));
         }
     }
 
@@ -3243,10 +3242,10 @@ bool ProjectedCRS::_isEquivalentTo(
 ProjectedCRSNNPtr
 ProjectedCRS::alterParametersLinearUnit(const common::UnitOfMeasure &unit,
                                         bool convertToNewUnit) const {
-    return create(createPropertyMap(this), baseCRS(),
-                  derivingConversionRef()->alterParametersLinearUnit(
-                      unit, convertToNewUnit),
-                  coordinateSystem());
+    return create(
+        createPropertyMap(this), baseCRS(),
+        derivingConversion()->alterParametersLinearUnit(unit, convertToNewUnit),
+        coordinateSystem());
 }
 //! @endcond
 
diff --git a/test/unit/test_crs.cpp b/test/unit/test_crs.cpp
index 0df128e6..faa1ace4 100644
--- a/test/unit/test_crs.cpp
+++ b/test/unit/test_crs.cpp
@@ -5281,3 +5281,70 @@ TEST(crs, promoteTo3D) {
         EXPECT_EQ(baseCRS->coordinateSystem()->axisList().size(), 3U);
     }
 }
+
+// ---------------------------------------------------------------------------
+
+TEST(crs, projected_normalizeForVisualization_do_not_mess_deriving_conversion) {
+
+    auto authFactory =
+        AuthorityFactory::create(DatabaseContext::create(), "EPSG");
+    // Something with non standard order
+    auto projCRS = authFactory->createProjectedCRS("3035");
+    {
+        auto src = GeographicCRS::EPSG_4326;
+        auto op =
+            CoordinateOperationFactory::create()->createOperation(src, projCRS);
+        ASSERT_TRUE(op != nullptr);
+        // Make sure to run that in a scope, so that the object get destroyed
+        op->normalizeForVisualization();
+    }
+    EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get());
+}
+
+// ---------------------------------------------------------------------------
+
+TEST(crs, projected_promoteTo3D_do_not_mess_deriving_conversion) {
+
+    auto projCRS = createProjected();
+    {
+        // Make sure to run that in a scope, so that the object get destroyed
+        projCRS->promoteTo3D(std::string(), nullptr);
+    }
+    EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get());
+}
+
+// ---------------------------------------------------------------------------
+
+TEST(crs, projected_alterGeodeticCRS_do_not_mess_deriving_conversion) {
+
+    auto projCRS = createProjected();
+    {
+        // Make sure to run that in a scope, so that the object get destroyed
+        projCRS->alterGeodeticCRS(NN_NO_CHECK(projCRS->extractGeographicCRS()));
+    }
+    EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get());
+}
+
+// ---------------------------------------------------------------------------
+
+TEST(crs, projected_alterCSLinearUnit_do_not_mess_deriving_conversion) {
+
+    auto projCRS = createProjected();
+    {
+        // Make sure to run that in a scope, so that the object get destroyed
+        projCRS->alterCSLinearUnit(UnitOfMeasure("my unit", 2));
+    }
+    EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get());
+}
+
+// ---------------------------------------------------------------------------
+
+TEST(crs, projected_alterParametersLinearUnit_do_not_mess_deriving_conversion) {
+
+    auto projCRS = createProjected();
+    {
+        // Make sure to run that in a scope, so that the object get destroyed
+        projCRS->alterParametersLinearUnit(UnitOfMeasure::METRE, false);
+    }
+    EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get());
+}