Repository: crawler-commons/crawler-commons (java)

Issue:
Allow empty price in video sitemaps
The [video sitemap schema](https://www.google.com/schemas/sitemap-video/1.1/sitemap-video.xsd) allows "the price value ... be empty." With an empty price, it's possible to express whether the video is for rent or purchase, e.g. `<video:price type="purchase" />`. This information should be added to the video attributes, as of now <video:price> elements with empty value are ignored. See also #220 which makes the price value optional in case the number cannot be parsed.

Pull request description:
Allow empty price in video sitemaps, fixes #221
Fix for #221 including unit test.
It's based on and includes PR #220.

Patch:
diff --git a/src/main/java/crawlercommons/sitemaps/extension/VideoAttributes.java b/src/main/java/crawlercommons/sitemaps/extension/VideoAttributes.java
index 2f6e810..b625946 100644
--- a/src/main/java/crawlercommons/sitemaps/extension/VideoAttributes.java
+++ b/src/main/java/crawlercommons/sitemaps/extension/VideoAttributes.java
@@ -424,17 +424,17 @@ public class VideoAttributes extends ExtensionMetadata {
         /**
          * Video price
          */
-        private float price;
+        private Float price;
 
-        public VideoPrice(final String currency, final float price) {
+        public VideoPrice(String currency, Float price) {
             this(currency, price, VideoPriceType.own);
         }
 
-        public VideoPrice(final String currency, final float price, final VideoPriceType type) {
+        public VideoPrice(String currency, Float price, VideoPriceType type) {
             this(currency, price, type, null);
         }
 
-        public VideoPrice(final String currency, final float price, final VideoPriceType type, final VideoPriceResolution resolution) {
+        public VideoPrice(String currency, Float price, VideoPriceType type, VideoPriceResolution resolution) {
             this.currency = currency;
             this.price = price;
             this.type = type;
@@ -456,7 +456,7 @@ public class VideoAttributes extends ExtensionMetadata {
             }
             VideoPrice that = (VideoPrice) other;
             return Objects.equals(currency, that.currency) //
-                            && price == that.price //
+                            && Objects.equals(price, that.price) //
                             && type == that.type //
                             && Objects.equals(resolution, that.resolution);
         }
@@ -473,11 +473,11 @@ public class VideoAttributes extends ExtensionMetadata {
             return resolution;
         }
 
-        public float getPrice() {
+        public Float getPrice() {
             return price;
         }
 
-        public void setPrice(float price) {
+        public void setPrice(Float price) {
             this.price = price;
         }
 
@@ -486,7 +486,7 @@ public class VideoAttributes extends ExtensionMetadata {
     public VideoAttributes() {
     }
 
-    public VideoAttributes(final URL thumbnailLoc, final String title, final String description, final URL contentLoc, final URL playerLoc) {
+    public VideoAttributes(URL thumbnailLoc, String title, String description, URL contentLoc, URL playerLoc) {
         this.thumbnailLoc = thumbnailLoc;
         this.title = title;
         this.description = description;
diff --git a/src/main/java/crawlercommons/sitemaps/sax/extension/VideoHandler.java b/src/main/java/crawlercommons/sitemaps/sax/extension/VideoHandler.java
index 7405f98..1378785 100644
--- a/src/main/java/crawlercommons/sitemaps/sax/extension/VideoHandler.java
+++ b/src/main/java/crawlercommons/sitemaps/sax/extension/VideoHandler.java
@@ -101,7 +101,36 @@ public class VideoHandler extends ExtensionHandler {
         if (currAttr == null) {
             currAttr = new VideoAttributes();
         }
-        if (value.isEmpty()) {
+        if ("price".equals(localName)) {
+            Float fvalue = getFloatValue(value);
+            String currency = null;
+            VideoPriceType type = VideoPriceType.own;
+            VideoPriceResolution resolution = null;
+            if (priceAttr != null) {
+                if (priceAttr.containsKey("currency")) {
+                    currency = priceAttr.get("currency").trim();
+                }
+                String t = priceAttr.get("type");
+                if (t != null && !t.trim().isEmpty()) {
+                    try {
+                        type = VideoPriceType.valueOf(t.trim());
+                    } catch (IllegalArgumentException e) {
+                        LOG.debug("Illegal value for price type: {}", type);
+                    }
+                }
+                String r = priceAttr.get("resolution");
+                if (r != null && !r.trim().isEmpty()) {
+                    try {
+                        resolution = VideoPriceResolution.valueOf(r.trim());
+                    } catch (IllegalArgumentException e) {
+                        LOG.debug("Illegal value for price resolution: {}", resolution);
+                    }
+                }
+            }
+            VideoPrice price = new VideoPrice(currency, fvalue, type, resolution);
+            currAttr.addPrice(price);
+            priceAttr = null;
+        } else if (value.isEmpty()) {
             // skip value but reset StringBuilder
         } else if ("thumbnail_loc".equals(localName)) {
             currAttr.setThumbnailLoc(getURLValue(value));
@@ -147,33 +176,6 @@ public class VideoHandler extends ExtensionHandler {
             relationAttr = null;
         } else if ("gallery_loc".equals(localName)) {
             currAttr.setGalleryLoc(getURLValue(value));
-        } else if ("price".equals(localName)) {
-            float fvalue = getFloatValue(value);
-            String currency = null;
-            VideoPriceType type = VideoPriceType.own;
-            VideoPriceResolution resolution = null;
-            if (priceAttr != null) {
-                currency = priceAttr.get("currency").trim();
-                String t = priceAttr.get("type");
-                if (t != null && !t.trim().isEmpty()) {
-                    try {
-                        type = VideoPriceType.valueOf(t.trim());
-                    } catch (IllegalArgumentException e) {
-                        LOG.debug("Illegal value for price type: {}", type);
-                    }
-                }
-                String r = priceAttr.get("resolution");
-                if (r != null && !r.trim().isEmpty()) {
-                    try {
-                        resolution = VideoPriceResolution.valueOf(r.trim());
-                    } catch (IllegalArgumentException e) {
-                        LOG.debug("Illegal value for price resolution: {}", resolution);
-                    }
-                }
-            }
-            VideoPrice price = new VideoPrice(currency, fvalue, type, resolution);
-            currAttr.addPrice(price);
-            priceAttr = null;
         } else if ("requires_subscription".equals(localName)) {
             currAttr.setRequiresSubscription(getYesNoBooleanValue(value, localName));
         } else if ("uploader".equals(localName)) {


Test patch:
diff --git a/src/test/java/crawlercommons/sitemaps/SiteMapParserExtensionTest.java b/src/test/java/crawlercommons/sitemaps/SiteMapParserExtensionTest.java
index b09ac66..283164c 100644
--- a/src/test/java/crawlercommons/sitemaps/SiteMapParserExtensionTest.java
+++ b/src/test/java/crawlercommons/sitemaps/SiteMapParserExtensionTest.java
@@ -25,6 +25,7 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.time.ZonedDateTime;
 import java.util.HashMap;
+import java.util.Iterator;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -54,7 +55,10 @@ public class SiteMapParserExtensionTest {
         assertEquals(false, asm.isIndex());
         assertEquals(true, asm instanceof SiteMap);
         SiteMap sm = (SiteMap) asm;
-        assertEquals(1, sm.getSiteMapUrls().size());
+        assertEquals(3, sm.getSiteMapUrls().size());
+        Iterator<SiteMapURL> siter = sm.getSiteMapUrls().iterator();
+
+        // first <loc> element: nearly all video attributes
         VideoAttributes expectedVideoAttributes = new VideoAttributes(new URL("http://www.example.com/thumbs/123.jpg"), "Grilling steaks for summer",
                         "Alkis shows you how to get perfectly done steaks every time", new URL("http://www.example.com/video123.flv"), new URL("http://www.example.com/videoplayer.swf?video=123"));
         expectedVideoAttributes.setDuration(600);
@@ -74,12 +78,28 @@ public class SiteMapParserExtensionTest {
         expectedVideoAttributes.setUploader("GrillyMcGrillerson");
         expectedVideoAttributes.setUploaderInfo(new URL("http://www.example.com/users/grillymcgrillerson"));
         expectedVideoAttributes.setLive(false);
+        VideoAttributes attr = (VideoAttributes) siter.next().getAttributesForExtension(Extension.VIDEO)[0];
+        assertNotNull(attr);
+        assertEquals(expectedVideoAttributes, attr);
 
-        for (SiteMapURL su : sm.getSiteMapUrls()) {
-            assertNotNull(su.getAttributesForExtension(Extension.VIDEO));
-            VideoAttributes attr = (VideoAttributes) su.getAttributesForExtension(Extension.VIDEO)[0];
-            assertEquals(expectedVideoAttributes, attr);
-        }
+        // locale-specific number format in <video:price>, test #220
+        // The current expected behavior is to not handle non-US locale price
+        // values and set the price value to null if parsing as float value
+        // fails.
+        expectedVideoAttributes = new VideoAttributes(new URL("http://www.example.com/thumbs/123-2.jpg"), "Grilling steaks for summer, episode 2",
+                        "Alkis shows you how to get perfectly done steaks every time", new URL("http://www.example.com/video123-2.flv"), null);
+        expectedVideoAttributes.setPrices(new VideoAttributes.VideoPrice[] { new VideoAttributes.VideoPrice("EUR", null, VideoAttributes.VideoPriceType.own) });
+        attr = (VideoAttributes) siter.next().getAttributesForExtension(Extension.VIDEO)[0];
+        assertNotNull(attr);
+        assertEquals(expectedVideoAttributes, attr);
+
+        // empty price, only type (purchase or rent) is indicated, see #221
+        expectedVideoAttributes = new VideoAttributes(new URL("http://www.example.com/thumbs/123-3.jpg"), "Grilling steaks for summer, episode 3",
+                        "Alkis shows you how to get perfectly done steaks every time", new URL("http://www.example.com/video123-3.flv"), null);
+        expectedVideoAttributes.setPrices(new VideoAttributes.VideoPrice[] { new VideoAttributes.VideoPrice(null, null, VideoAttributes.VideoPriceType.rent) });
+        attr = (VideoAttributes) siter.next().getAttributesForExtension(Extension.VIDEO)[0];
+        assertNotNull(attr);
+        assertEquals(expectedVideoAttributes, attr);
     }
 
     @Test
diff --git a/src/test/resources/sitemaps/extension/sitemap-videos.xml b/src/test/resources/sitemaps/extension/sitemap-videos.xml
index ad8415c..425ba64 100644
--- a/src/test/resources/sitemaps/extension/sitemap-videos.xml
+++ b/src/test/resources/sitemaps/extension/sitemap-videos.xml
@@ -17,9 +17,9 @@
       <video:rating>4.2</video:rating>
       <video:view_count>12345</video:view_count>
       <video:publication_date>2007-11-05T19:20:30+08:00</video:publication_date>
-      <video:family_friendly>yes</video:family_friendly>
       <video:tag>sample_tag1</video:tag>
       <video:tag>sample_tag2</video:tag>
+      <video:family_friendly>yes</video:family_friendly>
       <video:restriction relationship="allow">IE GB US CA</video:restriction>
       <video:gallery_loc title="Cooking Videos">http://cooking.example.com</video:gallery_loc>
       <video:price currency="EUR">1.99</video:price>
@@ -30,4 +30,28 @@
       <video:live>no</video:live>
     </video:video>
   </url>
+  <url>
+    <loc>http://www.example.com/videos/some_video_landing_page-2.html</loc>
+    <video:video>
+      <video:thumbnail_loc>http://www.example.com/thumbs/123-2.jpg</video:thumbnail_loc>
+      <video:title>Grilling steaks for summer, episode 2</video:title>
+      <video:description>Alkis shows you how to get perfectly done steaks every
+        time</video:description>
+      <video:content_loc>http://www.example.com/video123-2.flv</video:content_loc>
+      <!-- locale-specific number format, test #220 -->
+      <video:price currency="EUR">1,99</video:price>
+    </video:video>
+  </url>
+  <url>
+    <loc>http://www.example.com/videos/some_video_landing_page-3.html</loc>
+    <video:video>
+      <video:thumbnail_loc>http://www.example.com/thumbs/123-3.jpg</video:thumbnail_loc>
+      <video:title>Grilling steaks for summer, episode 3</video:title>
+      <video:description>Alkis shows you how to get perfectly done steaks every
+        time</video:description>
+      <video:content_loc>http://www.example.com/video123-3.flv</video:content_loc>
+      <!-- locale-specific number format, test #220 -->
+      <video:price type="rent"></video:price>
+    </video:video>
+  </url>
 </urlset>