diff --git a/editor/xml_feature.hpp b/editor/xml_feature.hpp index 720ed81e68..5b1c54d99e 100644 --- a/editor/xml_feature.hpp +++ b/editor/xml_feature.hpp @@ -48,6 +48,8 @@ public: XMLFeature(pugi::xml_document const & xml); XMLFeature(pugi::xml_node const & xml); XMLFeature(XMLFeature const & feature); + // TODO: It should make "deep" compare instead of converting to strings. + // Strings comparison does not work if tags order is different but tags are equal. bool operator==(XMLFeature const & other) const; /// @returns nodes and ways from osmXml. Vector can be empty. static vector FromOSM(string const & osmXml); diff --git a/indexer/feature.cpp b/indexer/feature.cpp index 9d17b25c67..a90d83e57f 100644 --- a/indexer/feature.cpp +++ b/indexer/feature.cpp @@ -105,7 +105,7 @@ void FeatureType::ReplaceBy(osm::EditableMapObject const & emo) m_id = emo.GetID(); } -editor::XMLFeature FeatureType::ToXML() const +editor::XMLFeature FeatureType::ToXML(bool serializeType) const { editor::XMLFeature feature(GetFeatureType() == feature::GEOM_POINT ? editor::XMLFeature::Type::Node @@ -139,32 +139,35 @@ editor::XMLFeature FeatureType::ToXML() const // feature.m_params.layer = // feature.m_params.rank = - feature::TypesHolder th(*this); - // TODO(mgsergio): Use correct sorting instead of SortBySpec based on the config. - th.SortBySpec(); - Classificator & cl = classif(); - // TODO(mgsergio): Either improve "OSM"-compatible serialization for more complex types, - // or save all our types directly, to restore and reuse them in migration of modified features. - for (uint32_t const type : th) + if (serializeType) { - string const strType = cl.GetReadableObjectName(type); - strings::SimpleTokenizer iter(strType, "-"); - string const k = *iter; - if (++iter) + feature::TypesHolder th(*this); + // TODO(mgsergio): Use correct sorting instead of SortBySpec based on the config. + th.SortBySpec(); + Classificator & cl = classif(); + // TODO(mgsergio): Either improve "OSM"-compatible serialization for more complex types, + // or save all our types directly, to restore and reuse them in migration of modified features. + for (uint32_t const type : th) { - // First (main) type is always stored as "k=amenity v=restaurant". - // Any other "k=amenity v=atm" is replaced by "k=atm v=yes". - if (feature.GetTagValue(k).empty()) - feature.SetTagValue(k, *iter); + string const strType = cl.GetReadableObjectName(type); + strings::SimpleTokenizer iter(strType, "-"); + string const k = *iter; + if (++iter) + { + // First (main) type is always stored as "k=amenity v=restaurant". + // Any other "k=amenity v=atm" is replaced by "k=atm v=yes". + if (feature.GetTagValue(k).empty()) + feature.SetTagValue(k, *iter); + else + feature.SetTagValue(*iter, "yes"); + } else - feature.SetTagValue(*iter, "yes"); - } - else - { - // We're editing building, generic craft, shop, office, amenity etc. - // Skip it's serialization. - // TODO(mgsergio): Correcly serialize all types back and forth. - LOG(LDEBUG, ("Skipping type serialization:", k)); + { + // We're editing building, generic craft, shop, office, amenity etc. + // Skip it's serialization. + // TODO(mgsergio): Correcly serialize all types back and forth. + LOG(LDEBUG, ("Skipping type serialization:", k)); + } } } diff --git a/indexer/feature.hpp b/indexer/feature.hpp index 495bbeb97e..d25a1d287a 100644 --- a/indexer/feature.hpp +++ b/indexer/feature.hpp @@ -170,7 +170,10 @@ public: /// Replaces all FeatureType's components. void ReplaceBy(osm::EditableMapObject const & ef); - editor::XMLFeature ToXML() const; + /// @param serializeType if false, types are not serialized. + /// Useful for applying modifications to existing OSM features, to avoid ussues when someone + /// has changed a type in OSM, but our users uploaded invalid outdated type after modifying feature. + editor::XMLFeature ToXML(bool serializeType) const; /// Creates new feature, including geometry and types. /// @Note: only nodes (points) are supported at the moment. bool FromXML(editor::XMLFeature const & xml); diff --git a/indexer/indexer_tests/feature_xml_test.cpp b/indexer/indexer_tests/feature_xml_test.cpp index 40651567cd..2d8008b3bc 100644 --- a/indexer/indexer_tests/feature_xml_test.cpp +++ b/indexer/indexer_tests/feature_xml_test.cpp @@ -3,10 +3,6 @@ #include "indexer/classificator_loader.hpp" #include "indexer/feature.hpp" -#include "base/string_utils.hpp" - -#include "std/sstream.hpp" -/* namespace { struct TestSetUp @@ -15,77 +11,33 @@ struct TestSetUp }; TestSetUp g_testSetUp; - -// Sort tags and compare as strings. -void CompareFeatureXML(string const & d1, string const & d2) -{ - pugi::xml_document xml1, xml2; - xml1.load(d1.data()); - xml2.load(d2.data()); - - xml1.child("node").remove_attribute("timestamp"); - xml2.child("node").remove_attribute("timestamp"); - - stringstream ss1, ss2; - xml1.save(ss1); - xml2.save(ss2); - - vector v1(strings::SimpleTokenizer(ss1.str(), "\n"), strings::SimpleTokenizer()); - vector v2(strings::SimpleTokenizer(ss2.str(), "\n"), strings::SimpleTokenizer()); - - TEST(v1.size() >= 3, ()); - TEST(v2.size() >= 3, ()); - - // Sort all except , and - sort(begin(v1) + 2, end(v1) - 1); - sort(begin(v2) + 2, end(v2) - 1); - - // Format back to string to have a nice readable error message. - auto s1 = strings::JoinStrings(v1, "\n"); - auto s2 = strings::JoinStrings(v2, "\n"); - - TEST_EQUAL(s1, s2, ()); -} } // namespace - TODO(mgsergio): Unkomment when creation is required. - UNIT_TEST(FeatureType_FromXMLAndBackToXML) - { - auto const xml = R"( - - - - - - - - - - )"; - auto const feature = FeatureType::FromXML(xml); - auto const xmlFeature = feature.ToXML(); +UNIT_TEST(FeatureType_FromXMLAndBackToXML) +{ + string const xmlNoTypeStr = R"( + + + + + + +)"; - stringstream sstr; - xmlFeature.Save(sstr); + char const kTimestamp[] = "2015-11-27T21:13:32Z"; - CompareFeatureXML(xml, sstr.str()); - } -*/ + editor::XMLFeature xmlNoType(xmlNoTypeStr); + editor::XMLFeature xmlWithType = xmlNoType; + xmlWithType.SetTagValue("amenity", "atm"); + + FeatureType ft; + ft.FromXML(xmlWithType); + auto fromFtWithType = ft.ToXML(true); + fromFtWithType.SetAttribute("timestamp", kTimestamp); + TEST_EQUAL(fromFtWithType, xmlWithType, ()); + + auto fromFtWithoutType = ft.ToXML(false); + fromFtWithoutType.SetAttribute("timestamp", kTimestamp); + TEST_EQUAL(fromFtWithoutType, xmlNoType, ()); +} diff --git a/indexer/osm_editor.cpp b/indexer/osm_editor.cpp index d0d34c57be..486ed029a2 100644 --- a/indexer/osm_editor.cpp +++ b/indexer/osm_editor.cpp @@ -277,7 +277,7 @@ bool Editor::Save(string const & fullFilePath) const { FeatureTypeInfo const & fti = index.second; // TODO: Do we really need to serialize deleted features in full details? Looks like mwm ID and meta fields are enough. - XMLFeature xf = fti.m_feature.ToXML(); + XMLFeature xf = fti.m_feature.ToXML(true /*type serializing helps during migration*/); xf.SetMWMFeatureIndex(index.first); if (!fti.m_street.empty()) xf.SetTagValue(kAddrStreetTag, fti.m_street); @@ -583,18 +583,17 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset try { - XMLFeature feature = fti.m_feature.ToXML(); - if (!fti.m_street.empty()) - feature.SetTagValue(kAddrStreetTag, fti.m_street); - - ourDebugFeatureString = DebugPrint(feature); - switch (fti.m_status) { case FeatureStatus::Untouched: CHECK(false, ("It's impossible.")); continue; case FeatureStatus::Created: { + XMLFeature feature = fti.m_feature.ToXML(true); + if (!fti.m_street.empty()) + feature.SetTagValue(kAddrStreetTag, fti.m_street); + ourDebugFeatureString = DebugPrint(feature); + ASSERT_EQUAL(feature.GetType(), XMLFeature::Type::Node, ("Linear and area features creation is not supported yet.")); try @@ -624,7 +623,7 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset } catch (...) { - // Pas network or other errors to outside exception handler. + // Pass network or other errors to outside exception handler. throw; } } @@ -632,6 +631,13 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset case FeatureStatus::Modified: { + // Do not serialize feature's type to avoid breaking OSM data. + // TODO: Implement correct types matching when we support modifying existing feature types. + XMLFeature feature = fti.m_feature.ToXML(false); + if (!fti.m_street.empty()) + feature.SetTagValue(kAddrStreetTag, fti.m_street); + ourDebugFeatureString = DebugPrint(feature); + XMLFeature osmFeature = GetMatchingFeatureFromOSM(changeset, m_getOriginalFeatureFn(fti.m_feature.GetID())); XMLFeature const osmFeatureCopy = osmFeature;