PpdProvider v3: update Printers metadata parser

This change updates the parsing code for Printers metadata to
accommodate the new version we are pushing from our serving root.

Bug: chromium:888189,b:161547656
Test: chromeos_unittests --gtest_filter='PpdMetadataParserTest.*'
Test: chromeos_unittests --gtest_filter='PpdMetadataManagerTest.*'
Change-Id: I54d3cbffdbbc3e51f4e636d0a904179a05c9132f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2315158Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Kalvin Lee <kdlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792959}

chromeos/printing/ppd_metadata_manager_unittest.cc

@@ -421,10 +421,13 @@ TEST_F(PpdMetadataManagerTest, CanGetPrinters) {
   GetFakeCache()->SetFetchResponseForTesting(
       "metadata_v3/Manufacturer_A-en.json", R"(
       {
-        "modelToEmm": {
-          "Some Printer A": "some emm a",
-          "Some Printer B": "some emm b"
-        }
+        "printers": [ {
+          "emm": "some emm a",
+          "name": "Some Printer A"
+        }, {
+          "emm": "some emm b",
+          "name": "Some Printer B"
+        } ]
       }
   )");
 

chromeos/printing/ppd_metadata_matchers.h

@@ -7,6 +7,10 @@
 
 #include <string>
 
+#include "base/strings/string_number_conversions.h"
+#include "base/version.h"
+#include "chromeos/printing/ppd_metadata_parser.h"
+#include "chromeos/printing/ppd_provider.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 
 #ifndef CHROMEOS_PRINTING_PPD_METADATA_MATCHERS_H_
@@ -14,10 +18,47 @@
 
 namespace chromeos {
 
+using ::testing::Eq;
 using ::testing::ExplainMatchResult;
 using ::testing::Field;
 using ::testing::StrEq;
 
+MATCHER_P(RestrictionsWithMinMilestone,
+          integral_min_milestone,
+          "is a Restrictions with min_milestone ``" +
+              base::NumberToString(integral_min_milestone) + "''") {
+  return ExplainMatchResult(Field(&PpdProvider::Restrictions::min_milestone,
+                                  Eq(base::Version(base::NumberToString(
+                                      double{integral_min_milestone})))),
+                            arg, result_listener);
+}
+
+MATCHER_P(RestrictionsWithMaxMilestone,
+          integral_max_milestone,
+          "is a Restrictions with max_milestone ``" +
+              base::NumberToString(integral_max_milestone) + "''") {
+  return ExplainMatchResult(Field(&PpdProvider::Restrictions::max_milestone,
+                                  Eq(base::Version(base::NumberToString(
+                                      double{integral_max_milestone})))),
+                            arg, result_listener);
+}
+
+MATCHER_P2(RestrictionsWithMinAndMaxMilestones,
+           integral_min_milestone,
+           integral_max_milestone,
+           "is a Restrictions with min_milestone ``" +
+               base::NumberToString(integral_min_milestone) +
+               "'' "
+               "and max_milestone ``" +
+               base::NumberToString(integral_max_milestone) + "''") {
+  return ExplainMatchResult(
+             RestrictionsWithMinMilestone(integral_min_milestone), arg,
+             result_listener) &&
+         ExplainMatchResult(
+             RestrictionsWithMaxMilestone(integral_max_milestone), arg,
+             result_listener);
+}
+
 // Matches a ReverseIndexLeaf struct against its |manufacturer| and
 // |model| members.
 MATCHER_P2(ReverseIndexLeafLike,

chromeos/printing/ppd_metadata_parser.cc

@@ -11,6 +11,7 @@
 #include "base/json/json_reader.h"
 #include "base/notreached.h"
 #include "base/optional.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/values.h"
 
@@ -58,8 +59,74 @@ base::Optional<base::Value> ParseJsonAndUnnestKey(
   return unnested;
 }
 
+// Returns a well-formed Restrictions struct from a dictionary |value|.
+// A well-formed Restrictions struct has at least one member that
+// for which calling base::Version::IsValid() will evaluate true.
+base::Optional<PpdProvider::Restrictions> ParseRestrictionsFromValue(
+    const base::Value& value) {
+  auto min_as_double = value.FindDoubleKey("minMilestone");
+  auto max_as_double = value.FindDoubleKey("maxMilestone");
+  if (!min_as_double.has_value() && !max_as_double.has_value()) {
+    return base::nullopt;
+  }
+
+  // While we don't want to deliberately store trivial base::Version
+  // members into the Restrictions struct, take heed that a
+  // calling IsValid() on a default-constructed base::Version returns
+  // false.
+  PpdProvider::Restrictions restrictions;
+  bool restrictions_is_nontrivial = false;
+  if (min_as_double.has_value()) {
+    base::Version min_milestone =
+        base::Version(base::NumberToString(int{min_as_double.value()}));
+    if (min_milestone.IsValid()) {
+      restrictions.min_milestone = min_milestone;
+      restrictions_is_nontrivial = true;
+    }
+  }
+  if (max_as_double.has_value()) {
+    base::Version max_milestone =
+        base::Version(base::NumberToString(int{max_as_double.value()}));
+    if (max_milestone.IsValid()) {
+      restrictions.max_milestone = max_milestone;
+      restrictions_is_nontrivial = true;
+    }
+  }
+
+  if (restrictions_is_nontrivial) {
+    return restrictions;
+  }
+  return base::nullopt;
+}
+
+// Returns a ParsedPrinter from a leaf |value| from Printers metadata.
+base::Optional<ParsedPrinter> ParsePrinterFromValue(const base::Value& value) {
+  const std::string* const effective_make_and_model =
+      value.FindStringKey("emm");
+  const std::string* const name = value.FindStringKey("name");
+  if (!effective_make_and_model || effective_make_and_model->empty() || !name ||
+      name->empty()) {
+    return base::nullopt;
+  }
+  ParsedPrinter printer;
+  printer.effective_make_and_model = *effective_make_and_model;
+  printer.user_visible_printer_name = *name;
+
+  const base::Value* const restrictions_value =
+      value.FindDictKey("restriction");
+  if (restrictions_value) {
+    printer.restrictions = ParseRestrictionsFromValue(*restrictions_value);
+  }
+  return printer;
+}
+
 }  // namespace
 
+ParsedPrinter::ParsedPrinter() = default;
+ParsedPrinter::~ParsedPrinter() = default;
+ParsedPrinter::ParsedPrinter(const ParsedPrinter&) = default;
+ParsedPrinter& ParsedPrinter::operator=(const ParsedPrinter&) = default;
+
 base::Optional<std::vector<std::string>> ParseLocales(
     base::StringPiece locales_json) {
   const auto as_value =
@@ -105,20 +172,23 @@ base::Optional<ParsedManufacturers> ParseManufacturers(
 }
 
 base::Optional<ParsedPrinters> ParsePrinters(base::StringPiece printers_json) {
-  const auto as_value = ParseJsonAndUnnestKey(printers_json, "modelToEmm",
-                                              base::Value::Type::DICTIONARY);
-
+  const auto as_value =
+      ParseJsonAndUnnestKey(printers_json, "printers", base::Value::Type::LIST);
   if (!as_value.has_value()) {
     return base::nullopt;
   }
+
   ParsedPrinters printers;
-  for (const auto& iter : as_value.value().DictItems()) {
-    std::string printer_effective_make_and_model;
-    if (!iter.second.GetAsString(&printer_effective_make_and_model)) {
+  for (const auto& printer_value : as_value->GetList()) {
+    if (!printer_value.is_dict()) {
+      continue;
+    }
+    base::Optional<ParsedPrinter> printer =
+        ParsePrinterFromValue(printer_value);
+    if (!printer.has_value()) {
       continue;
     }
-    printers.push_back(
-        ParsedPrinter{iter.first, printer_effective_make_and_model});
+    printers.push_back(printer.value());
   }
   if (printers.empty()) {
     return base::nullopt;

chromeos/printing/ppd_metadata_parser.h

@@ -22,21 +22,28 @@
 #include "base/optional.h"
 #include "base/strings/string_piece_forward.h"
 #include "chromeos/chromeos_export.h"
+#include "chromeos/printing/ppd_provider.h"
 
 #ifndef CHROMEOS_PRINTING_PPD_METADATA_PARSER_H_
 #define CHROMEOS_PRINTING_PPD_METADATA_PARSER_H_
 
 namespace chromeos {
 
-struct ReverseIndexLeaf {
+struct CHROMEOS_EXPORT ReverseIndexLeaf {
   std::string manufacturer;
   std::string model;
 };
 
 // A ParsedPrinter is a value parsed from printers metadata.
-struct ParsedPrinter {
+struct CHROMEOS_EXPORT ParsedPrinter {
+  ParsedPrinter();
+  ~ParsedPrinter();
+  ParsedPrinter(const ParsedPrinter&);
+  ParsedPrinter& operator=(const ParsedPrinter&);
+
   std::string user_visible_printer_name;
   std::string effective_make_and_model;
+  base::Optional<PpdProvider::Restrictions> restrictions;
 };
 
 // Maps manufacturer names to basenames of printers metadata.

chromeos/printing/ppd_metadata_parser_unittest.cc

@@ -13,9 +13,11 @@
 namespace chromeos {
 namespace {
 
+using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::ExplainMatchResult;
 using ::testing::Field;
+using ::testing::Optional;
 using ::testing::Pair;
 using ::testing::StrEq;
 using ::testing::UnorderedElementsAre;
@@ -154,10 +156,13 @@ TEST(PpdMetadataParserTest, ParseManufacturersFailsGracefully) {
 TEST(PpdMetadataParserTest, CanParsePrinters) {
   constexpr base::StringPiece kPrintersJson = R"(
   {
-    "modelToEmm": {
-      "An die Musik": "d 547b",
-      "Auf der Donau": "d 553"
-    }
+    "printers": [ {
+      "emm": "d 547b",
+      "name": "An die Musik"
+    }, {
+      "emm": "d 553",
+      "name": "Auf der Donau"
+    } ]
   }
   )";
 
@@ -172,18 +177,18 @@ TEST(PpdMetadataParserTest, CanParsePrinters) {
 // Verifies that ParsePrinters() can parse printers and return a partial
 // list even when it encounters unexpected values.
 TEST(PpdMetadataParserTest, CanPartiallyParsePrinters) {
-  // Contains an embedded dictionary keyed on "Dearie me."
-  // ParsePrinters() shall ignore this.
+  // Contains an extra value keyed on "hello" in an otherwise valid leaf
+  // value in Printers metadata. ParsePrinters() shall ignore this.
   constexpr base::StringPiece kPrintersJson = R"(
   {
-    "modelToEmm": {
-      "Dearie me": {
-        "I didn't": "expect",
-        "to go": "deeper"
-      },
-      "Hänflings Liebeswerbung": "d 552",
-      "Auf der Donau": "d 553"
-    }
+    "printers": [ {
+      "emm": "d 552",
+      "name": "Hänflings Liebeswerbung",
+      "hello": "there!"
+    }, {
+      "emm": "d 553",
+      "name": "Auf der Donau"
+    } ]
   }
   )";
 
@@ -196,24 +201,86 @@ TEST(PpdMetadataParserTest, CanPartiallyParsePrinters) {
                   ParsedPrinterLike("Auf der Donau", "d 553")));
 }
 
-// Verifies that ParsePrinters() returns base::nullopt rather than an
-// empty container.
-TEST(PpdMetadataParserTest, ParsePrintersDoesNotReturnEmptyContainer) {
-  // Contains an embedded dictionary keyed on "Dearie me."
-  // ParsePrinters() shall ignore this, but in doing so shall make the
-  // returned ParsedPrinters empty.
+// Verifies that ParsePrinters() can parse printers and their
+// well-formed restrictions (if any are specified).
+TEST(PpdMetadataParserTest, CanParsePrintersWithRestrictions) {
+  // Specifies
+  // *  a printer with a minimum milestone,
+  // *  a printer with a maximum milestone, and
+  // *  a printer with both minimum and maximum milestones.
   constexpr base::StringPiece kPrintersJson = R"(
   {
-    "modelToEmm": {
-      "Dearie me": {
-        "I didn't": "expect",
-        "to go": "deeper"
+    "printers": [ {
+      "emm": "d 121",
+      "name": "Schäfers Klagelied",
+      "restriction": {
+        "minMilestone": 121
       }
-    }
+    }, {
+      "emm": "d 216",
+      "name": "Meeres Stille",
+      "restriction": {
+        "maxMilestone": 216
+      }
+    }, {
+      "emm": "d 257",
+      "name": "Heidenröslein",
+      "restriction": {
+        "minMilestone": 216,
+        "maxMilestone": 257
+      }
+    } ]
   }
   )";
 
-  EXPECT_FALSE(ParsePrinters(kPrintersJson).has_value());
+  const auto parsed = ParsePrinters(kPrintersJson);
+  ASSERT_TRUE(parsed.has_value());
+
+  EXPECT_THAT(
+      *parsed,
+      UnorderedElementsAre(
+          AllOf(ParsedPrinterLike("Schäfers Klagelied", "d 121"),
+                Field(&ParsedPrinter::restrictions,
+                      Optional(RestrictionsWithMinMilestone(121)))),
+          AllOf(ParsedPrinterLike("Meeres Stille", "d 216"),
+                Field(&ParsedPrinter::restrictions,
+                      Optional(RestrictionsWithMaxMilestone(216)))),
+          AllOf(
+              ParsedPrinterLike("Heidenröslein", "d 257"),
+              Field(&ParsedPrinter::restrictions,
+                    Optional(RestrictionsWithMinAndMaxMilestones(216, 257))))));
+}
+
+// Verifies that ParsePrinters() can parse printers and ignore
+// malformed restrictions.
+TEST(PpdMetadataParserTest, CanParsePrintersWithMalformedRestrictions) {
+  // Specifies a printer with invalid restrictions.
+  constexpr base::StringPiece kPrintersJson = R"(
+  {
+    "printers": [ {
+      "emm": "d 368",
+      "name": "Jägers Abendlied",
+      "restriction": {
+        "hello": "there!"
+      }
+    } ]
+  }
+  )";
+
+  const auto parsed = ParsePrinters(kPrintersJson);
+  ASSERT_TRUE(parsed.has_value());
+
+  EXPECT_THAT(*parsed,
+              UnorderedElementsAre(AllOf(
+                  ParsedPrinterLike("Jägers Abendlied", "d 368"),
+                  Field(&ParsedPrinter::restrictions, Eq(base::nullopt)))));
+}
+
+// Verifies that ParsePrinters() returns base::nullopt rather than an
+// empty container.
+TEST(PpdMetadataParserTest, ParsePrintersDoesNotReturnEmptyContainer) {
+  // No printers are specified in this otherwise valid JSON dictionary.
+  EXPECT_FALSE(ParsePrinters("{}").has_value());
 }
 
 // Verifies that ParsePrinters() returns base::nullopt on irrecoverable

chromeos/printing/ppd_provider.h

@@ -107,6 +107,7 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
   };
 
   // Defines the limitations on when we show a particular PPD
+  // TODO(crbug.com/888189): this belongs in the parsing header.
   struct Restrictions {
     // Minimum milestone for ChromeOS build
     base::Version min_milestone = base::Version("0.0");