Found a few codec inconsistencies between JSON encoders and decoders

- Fix EtherType inconsistency in coding VLAN_PUSH and MPLS_POP instructions
- Fix group id inconsistency in coding a Group instruction
- Fix Group identifiers to be treated as unsigned integers in GroupCodec
- Add a JSON decoder for FlowEntry

Change-Id: Id80b9e009550e3b71b9274adb116e5179391fa66
(cherry picked from commit 494f586853c1a87ba62f20b477e211f996baded5)
diff --git a/core/common/src/main/java/org/onosproject/codec/impl/DecodeInstructionCodecHelper.java b/core/common/src/main/java/org/onosproject/codec/impl/DecodeInstructionCodecHelper.java
index 2730c38..2933ca9 100644
--- a/core/common/src/main/java/org/onosproject/codec/impl/DecodeInstructionCodecHelper.java
+++ b/core/common/src/main/java/org/onosproject/codec/impl/DecodeInstructionCodecHelper.java
@@ -476,8 +476,10 @@
             return Instructions.transition(nullIsIllegal(json.get(InstructionCodec.TABLE_ID),
                     InstructionCodec.TABLE_ID + InstructionCodec.MISSING_MEMBER_MESSAGE).asInt());
         } else if (type.equals(Instruction.Type.GROUP.name())) {
-            GroupId groupId = new GroupId(nullIsIllegal(json.get(InstructionCodec.GROUP_ID),
-                    InstructionCodec.GROUP_ID + InstructionCodec.MISSING_MEMBER_MESSAGE).asInt());
+            // a group id should be an unsigned integer
+            Long id = nullIsIllegal(json.get(InstructionCodec.GROUP_ID),
+                    InstructionCodec.GROUP_ID + InstructionCodec.MISSING_MEMBER_MESSAGE).asLong();
+            GroupId groupId = new GroupId(id.intValue());
             return Instructions.createGroup(groupId);
         } else if (type.equals(Instruction.Type.METER.name())) {
             MeterId meterId = MeterId.meterId(nullIsIllegal(json.get(InstructionCodec.METER_ID),
diff --git a/core/common/src/main/java/org/onosproject/codec/impl/EncodeInstructionCodecHelper.java b/core/common/src/main/java/org/onosproject/codec/impl/EncodeInstructionCodecHelper.java
index 0f3c125..a30068e 100644
--- a/core/common/src/main/java/org/onosproject/codec/impl/EncodeInstructionCodecHelper.java
+++ b/core/common/src/main/java/org/onosproject/codec/impl/EncodeInstructionCodecHelper.java
@@ -18,6 +18,7 @@
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import org.onlab.osgi.DefaultServiceDirectory;
 import org.onlab.osgi.ServiceDirectory;
+import org.onlab.packet.EthType;
 import org.onlab.util.HexString;
 import org.onosproject.codec.CodecContext;
 import org.onosproject.net.Device;
@@ -40,7 +41,6 @@
 import org.onosproject.net.pi.runtime.PiActionProfileMemberId;
 import org.slf4j.Logger;
 
-import static org.onlab.util.Tools.toHexWithPrefix;
 import static org.slf4j.LoggerFactory.getLogger;
 
 /**
@@ -145,7 +145,8 @@
             case VLAN_PUSH:
                 final L2ModificationInstruction.ModVlanHeaderInstruction pushVlanInstruction =
                         (L2ModificationInstruction.ModVlanHeaderInstruction) l2Instruction;
-                result.put(InstructionCodec.ETHERNET_TYPE, pushVlanInstruction.ethernetType().toString());
+                result.put(InstructionCodec.ETHERNET_TYPE,
+                            toHexEthernetType(pushVlanInstruction.ethernetType().toString()));
                 break;
             case VLAN_POP:
                 break;
@@ -174,7 +175,7 @@
                 final L2ModificationInstruction.ModMplsHeaderInstruction popHeaderInstruction =
                         (L2ModificationInstruction.ModMplsHeaderInstruction) l2Instruction;
                 result.put(InstructionCodec.ETHERNET_TYPE,
-                        toHexWithPrefix(popHeaderInstruction.ethernetType().toShort()));
+                           toHexEthernetType(popHeaderInstruction.ethernetType().toString()));
                 break;
             case DEC_MPLS_TTL:
                 break;
@@ -185,6 +186,16 @@
     }
 
     /**
+     * Encode ethernet types in the conventional way of 4-digit hex string.
+     * @param ethTypeStr
+     * @return EtherType representation
+     */
+    private String toHexEthernetType(String ethTypeStr) {
+        EthType ethType = EthType.EtherType.valueOf(ethTypeStr.toUpperCase()).ethType();
+        return String.format("0x%04x", ethType.toShort());
+    }
+
+    /**
      * Encode an L3 modification instruction.
      *
      * @param result json node that the instruction attributes are added to
@@ -329,7 +340,9 @@
             case GROUP:
                 final Instructions.GroupInstruction groupInstruction =
                         (Instructions.GroupInstruction) instruction;
-                result.put(InstructionCodec.GROUP_ID, groupInstruction.groupId().toString());
+                // a group id should be an unsigned integer
+                result.put(InstructionCodec.GROUP_ID,
+                           Integer.toUnsignedLong(groupInstruction.groupId().id()));
                 break;
 
             case METER:
diff --git a/core/common/src/main/java/org/onosproject/codec/impl/FlowEntryCodec.java b/core/common/src/main/java/org/onosproject/codec/impl/FlowEntryCodec.java
index f0a53e1..0ffeeb2 100644
--- a/core/common/src/main/java/org/onosproject/codec/impl/FlowEntryCodec.java
+++ b/core/common/src/main/java/org/onosproject/codec/impl/FlowEntryCodec.java
@@ -15,12 +15,13 @@
  */
 package org.onosproject.codec.impl;
 
+import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import org.onosproject.codec.CodecContext;
 import org.onosproject.codec.JsonCodec;
-import org.onosproject.core.ApplicationId;
-import org.onosproject.core.CoreService;
+import org.onosproject.net.flow.DefaultFlowEntry;
 import org.onosproject.net.flow.FlowEntry;
+import org.onosproject.net.flow.FlowRule;
 import org.onosproject.net.flow.TrafficSelector;
 import org.onosproject.net.flow.TrafficTreatment;
 
@@ -31,46 +32,74 @@
  */
 public final class FlowEntryCodec extends JsonCodec<FlowEntry> {
 
+    public static final String GROUP_ID = "groupId";
+    public static final String STATE = "state";
+    public static final String LIFE = "life";
+    public static final String LIVE_TYPE = "liveType";
+    public static final String LAST_SEEN = "lastSeen";
+    public static final String PACKETS = "packets";
+    public static final String BYTES = "bytes";
+    public static final String TREATMENT = "treatment";
+    public static final String SELECTOR = "selector";
+
     @Override
     public ObjectNode encode(FlowEntry flowEntry, CodecContext context) {
         checkNotNull(flowEntry, "Flow entry cannot be null");
 
-        CoreService service = context.getService(CoreService.class);
-
-        ApplicationId appId = service.getAppId(flowEntry.appId());
-
-        String strAppId = (appId == null) ? "<none>" : appId.name();
-
-        final ObjectNode result = context.mapper().createObjectNode()
-                .put("id", Long.toString(flowEntry.id().value()))
-                .put("tableId", flowEntry.table().toString())
-                .put("appId", strAppId)
-                .put("groupId", flowEntry.groupId().id())
-                .put("priority", flowEntry.priority())
-                .put("timeout", flowEntry.timeout())
-                .put("isPermanent", flowEntry.isPermanent())
-                .put("deviceId", flowEntry.deviceId().toString())
-                .put("state", flowEntry.state().toString())
-                .put("life", flowEntry.life()) //FIXME life is destroying precision (seconds granularity is default)
-                .put("packets", flowEntry.packets())
-                .put("bytes", flowEntry.bytes())
-                .put("liveType", flowEntry.liveType().toString())
-                .put("lastSeen", flowEntry.lastSeen());
+        ObjectNode result = context.mapper().createObjectNode()
+                .put(GROUP_ID, flowEntry.groupId().id())
+                .put(STATE, flowEntry.state().toString())
+                //FIXME life is destroying precision (seconds granularity is default)
+                .put(LIFE, flowEntry.life())
+                .put(LIVE_TYPE, flowEntry.liveType().toString())
+                .put(LAST_SEEN, flowEntry.lastSeen())
+                .put(PACKETS, flowEntry.packets())
+                .put(BYTES, flowEntry.bytes())
+                // encode FlowRule-specific fields using the FlowRule codec
+                .setAll(context.codec(FlowRule.class).encode((FlowRule) flowEntry, context));
 
         if (flowEntry.treatment() != null) {
             final JsonCodec<TrafficTreatment> treatmentCodec =
                     context.codec(TrafficTreatment.class);
-            result.set("treatment", treatmentCodec.encode(flowEntry.treatment(), context));
+            result.set(TREATMENT, treatmentCodec.encode(flowEntry.treatment(), context));
         }
 
         if (flowEntry.selector() != null) {
             final JsonCodec<TrafficSelector> selectorCodec =
                     context.codec(TrafficSelector.class);
-            result.set("selector", selectorCodec.encode(flowEntry.selector(), context));
+            result.set(SELECTOR, selectorCodec.encode(flowEntry.selector(), context));
         }
 
         return result;
     }
 
+    @Override
+    public FlowEntry decode(ObjectNode json, CodecContext context) {
+        checkNotNull(json, "JSON object cannot be null");
+
+        // decode FlowRule-specific fields using the FlowRule codec
+        FlowRule flowRule = context.codec(FlowRule.class).decode(json, context);
+
+        JsonNode stateNode = json.get(STATE);
+        FlowEntry.FlowEntryState state = (null == stateNode) ?
+                FlowEntry.FlowEntryState.ADDED : FlowEntry.FlowEntryState.valueOf(stateNode.asText());
+
+        JsonNode lifeNode = json.get(LIFE);
+        long life = (null == lifeNode) ? 0 : lifeNode.asLong();
+
+        JsonNode liveTypeNode = json.get(LIVE_TYPE);
+        FlowEntry.FlowLiveType liveType = (null == liveTypeNode) ?
+                FlowEntry.FlowLiveType.UNKNOWN : FlowEntry.FlowLiveType.valueOf(liveTypeNode.asText());
+
+        JsonNode packetsNode = json.get(PACKETS);
+        long packets = (null == packetsNode) ? 0 : packetsNode.asLong();
+
+        JsonNode bytesNode = json.get(BYTES);
+        long bytes = (null == bytesNode) ? 0 : bytesNode.asLong();
+
+        return new DefaultFlowEntry(flowRule, state, life,
+                                    liveType, packets, bytes);
+    }
+
 }
 
diff --git a/core/common/src/main/java/org/onosproject/codec/impl/GroupCodec.java b/core/common/src/main/java/org/onosproject/codec/impl/GroupCodec.java
index 37c0319..8955226 100644
--- a/core/common/src/main/java/org/onosproject/codec/impl/GroupCodec.java
+++ b/core/common/src/main/java/org/onosproject/codec/impl/GroupCodec.java
@@ -71,7 +71,8 @@
     public ObjectNode encode(Group group, CodecContext context) {
         checkNotNull(group, "Group cannot be null");
         ObjectNode result = context.mapper().createObjectNode()
-                .put(ID, group.id().id().toString())
+                // a Group id should be an unsigned integer
+                .put(ID, Integer.toUnsignedLong(group.id().id()))
                 .put(STATE, group.state().toString())
                 .put(LIFE, group.life())
                 .put(PACKETS, group.packets())
@@ -89,7 +90,8 @@
         }
 
         if (group.givenGroupId() != null) {
-            result.put(GIVEN_GROUP_ID, group.givenGroupId().toString());
+            // a given Group id should be an unsigned integer
+            result.put(GIVEN_GROUP_ID, Integer.toUnsignedLong(group.givenGroupId()));
         }
 
         ArrayNode buckets = context.mapper().createArrayNode();
@@ -110,10 +112,24 @@
         final JsonCodec<GroupBucket> groupBucketCodec = context.codec(GroupBucket.class);
         CoreService coreService = context.getService(CoreService.class);
 
-        // parse group id
-        int groupIdInt = nullIsIllegal(json.get(GROUP_ID),
-                GROUP_ID + MISSING_MEMBER_MESSAGE).asInt();
-        GroupId groupId = new GroupId(groupIdInt);
+        // parse uInt Group id from the ID field
+        JsonNode idNode = json.get(ID);
+        Long id = (null == idNode) ?
+                // use GROUP_ID for the corresponding Group id if ID is not supplied
+                nullIsIllegal(json.get(GROUP_ID), ID + MISSING_MEMBER_MESSAGE).asLong() :
+                idNode.asLong();
+        GroupId groupId = GroupId.valueOf(id.intValue());
+
+        // parse uInt Group id given by caller. see the GroupDescription javadoc
+        JsonNode givenGroupIdNode = json.get(GIVEN_GROUP_ID);
+        // if GIVEN_GROUP_ID is not supplied, set null so the group subsystem
+        // to choose the Group id (which is the value of ID indeed).
+        // else, must be same with ID to show both originate from the same source
+        Integer givenGroupIdInt = (null == givenGroupIdNode) ?
+                null : new Long(json.get(GIVEN_GROUP_ID).asLong()).intValue();
+        if (givenGroupIdInt != null && !givenGroupIdInt.equals(groupId.id())) {
+            throw new IllegalArgumentException(GIVEN_GROUP_ID + " must be same with " + ID);
+        }
 
         // parse group key (appCookie)
         String groupKeyStr = nullIsIllegal(json.get(APP_COOKIE),
@@ -157,7 +173,6 @@
         }
 
         // parse group buckets
-
         GroupBuckets buckets = null;
         List<GroupBucket> groupBucketList = new ArrayList<>();
         JsonNode bucketsJson = json.get(BUCKETS);
@@ -173,7 +188,7 @@
         }
 
         GroupDescription groupDescription = new DefaultGroupDescription(deviceId,
-                groupType, buckets, groupKey, groupIdInt, appId);
+                groupType, buckets, groupKey, givenGroupIdInt, appId);
 
         return new DefaultGroup(groupId, groupDescription);
     }
diff --git a/core/common/src/test/java/org/onosproject/codec/impl/FlowEntryCodecTest.java b/core/common/src/test/java/org/onosproject/codec/impl/FlowEntryCodecTest.java
new file mode 100644
index 0000000..b6204bb
--- /dev/null
+++ b/core/common/src/test/java/org/onosproject/codec/impl/FlowEntryCodecTest.java
@@ -0,0 +1,151 @@
+/*
+ * Copyright 2015-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.onosproject.codec.impl;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import org.junit.Before;
+import org.junit.Test;
+import org.onosproject.codec.JsonCodec;
+import org.onosproject.core.CoreService;
+import org.onosproject.net.DeviceId;
+import org.onosproject.net.PortNumber;
+import org.onosproject.net.flow.DefaultFlowEntry;
+import org.onosproject.net.flow.DefaultTrafficSelector;
+import org.onosproject.net.flow.DefaultTrafficTreatment;
+import org.onosproject.net.flow.FlowEntry;
+import org.onosproject.net.flow.FlowRule;
+import org.onosproject.net.flow.TrafficSelector;
+import org.onosproject.net.flow.TrafficTreatment;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import static org.easymock.EasyMock.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.onosproject.codec.impl.JsonCodecUtils.assertJsonEncodable;
+import static org.onosproject.net.NetTestTools.APP_ID;
+
+/**
+ * Unit test for FlowEntryCodec.
+ */
+public class FlowEntryCodecTest {
+
+    private static final FlowEntry.FlowEntryState STATE;
+    private static final long LIFE;
+    private static final FlowEntry.FlowLiveType LIVE_TYPE;
+    private static final long PACKETS;
+    private static final long BYTES;
+    private static final TrafficSelector SELECTOR;
+    private static final TrafficTreatment TREATMENT;
+    private static final FlowRule FLOW_RULE;
+    private static final FlowEntry FLOW_ENTRY;
+
+    private CoreService mockCoreService = createMock(CoreService.class);
+    private MockCodecContext context = new MockCodecContext();
+    private JsonCodec<FlowEntry> flowEntryCodec = context.codec(FlowEntry.class);
+
+    private static final String JSON_FILE = "simple-flow-entry.json";
+
+    static {
+        // make sure these members have same values with the corresponding JSON fields
+        STATE = FlowEntry.FlowEntryState.valueOf("ADDED");
+        LIFE = 1000;
+        LIVE_TYPE = FlowEntry.FlowLiveType.valueOf("UNKNOWN");
+        PACKETS = 123;
+        BYTES = 456;
+
+        SELECTOR = DefaultTrafficSelector.builder()
+                .matchEthType((short) (Integer.decode("0x800") & 0xFFFF))
+                .build();
+        TREATMENT = DefaultTrafficTreatment.builder()
+                .setOutput(PortNumber.fromString("CONTROLLER"))
+                .build();
+        FLOW_RULE = DefaultFlowEntry.builder()
+                .withCookie(1)
+                // isPermanent & timeout
+                .makeTemporary(1)
+                .forDevice(DeviceId.deviceId("of:0000000000000001"))
+                .forTable(Integer.valueOf("1"))
+                .withPriority(Integer.valueOf("1"))
+                .withSelector(SELECTOR)
+                .withTreatment(TREATMENT)
+                .build();
+        FLOW_ENTRY = new DefaultFlowEntry(FLOW_RULE, STATE, LIFE, LIVE_TYPE, PACKETS, BYTES);
+    }
+
+    @Before
+    public void setUp() {
+        expect(mockCoreService.registerApplication(FlowRuleCodec.REST_APP_ID))
+                .andReturn(APP_ID).anyTimes();
+        expect(mockCoreService.registerApplication(APP_ID.name()))
+                .andReturn(APP_ID).anyTimes();
+        expect(mockCoreService.getAppId(anyShort())).andReturn(APP_ID).anyTimes();
+        replay(mockCoreService);
+
+        context.registerService(CoreService.class, mockCoreService);
+    }
+
+    @Test
+    public void testCodec() {
+        assertNotNull(flowEntryCodec);
+        assertJsonEncodable(context, flowEntryCodec, FLOW_ENTRY);
+    }
+
+    @Test
+    public void testDecode() throws IOException {
+        InputStream jsonStream = FlowEntryCodec.class.getResourceAsStream(JSON_FILE);
+        JsonNode jsonString = context.mapper().readTree(jsonStream);
+
+        FlowEntry expected = flowEntryCodec.decode((ObjectNode) jsonString, context);
+        assertEquals(expected, FLOW_ENTRY);
+    }
+
+    @Test
+    public void testEncode() throws IOException {
+        InputStream jsonStream = FlowEntryCodec.class.getResourceAsStream(JSON_FILE);
+        ObjectNode jsonString = (ObjectNode) context.mapper().readTree(jsonStream);
+
+        ObjectNode expected = flowEntryCodec.encode(FLOW_ENTRY, context);
+        // only set by the internal FlowRule encoder, so should not appear in the JSON string
+        expected.remove("id");
+        expected.remove("appId");
+        expected.remove("tableName");
+
+        // only set by the FlowEntry encoder but not used for the decoder
+        // so should not appear in the JSON, or a decoding error occurs
+        expected.remove(FlowEntryCodec.GROUP_ID);
+        expected.remove(FlowEntryCodec.LAST_SEEN);
+
+        // assert equality of those values separately. see below
+        assertEquals(expected.get(FlowEntryCodec.LIFE).asLong(), jsonString.get(FlowEntryCodec.LIFE).asLong());
+        assertEquals(expected.get(FlowEntryCodec.PACKETS).asLong(), jsonString.get(FlowEntryCodec.PACKETS).asLong());
+        assertEquals(expected.get(FlowEntryCodec.BYTES).asLong(), jsonString.get(FlowEntryCodec.BYTES).asLong());
+
+        // if those numeric values are included in expected as a result of the encoding,
+        // AssertionError occurs even though both expected and jsonString are semantically identical
+        expected.remove(FlowEntryCodec.LIFE);
+        expected.remove(FlowEntryCodec.PACKETS);
+        expected.remove(FlowEntryCodec.BYTES);
+        jsonString.remove(FlowEntryCodec.LIFE);
+        jsonString.remove(FlowEntryCodec.PACKETS);
+        jsonString.remove(FlowEntryCodec.BYTES);
+
+        assertEquals(expected, jsonString);
+    }
+
+}
diff --git a/core/common/src/test/resources/org/onosproject/codec/impl/simple-flow-entry.json b/core/common/src/test/resources/org/onosproject/codec/impl/simple-flow-entry.json
new file mode 100644
index 0000000..b00ba8d
--- /dev/null
+++ b/core/common/src/test/resources/org/onosproject/codec/impl/simple-flow-entry.json
@@ -0,0 +1,29 @@
+{
+  "priority": 1,
+  "isPermanent": false,
+  "timeout": 1,
+  "deviceId": "of:0000000000000001",
+  "tableId": 1,
+  "treatment": {
+    "instructions": [
+      {
+        "type": "OUTPUT",
+        "port": "CONTROLLER"
+      }
+    ],
+    "deferred": []
+  },
+  "selector": {
+    "criteria": [
+      {
+        "type": "ETH_TYPE",
+        "ethType": "0x800"
+      }
+    ]
+  },
+  "state": "ADDED",
+  "life": 1000,
+  "liveType": "UNKNOWN",
+  "packets": 123,
+  "bytes": 456
+}
\ No newline at end of file
diff --git a/core/common/src/test/resources/org/onosproject/codec/impl/simple-group.json b/core/common/src/test/resources/org/onosproject/codec/impl/simple-group.json
index 865ae23..2229372 100644
--- a/core/common/src/test/resources/org/onosproject/codec/impl/simple-group.json
+++ b/core/common/src/test/resources/org/onosproject/codec/impl/simple-group.json
@@ -2,7 +2,7 @@
   "type": "ALL",
   "deviceId": "of:0000000000000001",
   "appCookie": "0x1234abCD",
-  "groupId": "1",
+  "id": "1",
   "buckets": [
     {
       "treatment": {
diff --git a/web/api/src/test/java/org/onosproject/rest/resources/GroupsResourceTest.java b/web/api/src/test/java/org/onosproject/rest/resources/GroupsResourceTest.java
index e13784d..3537c4d 100644
--- a/web/api/src/test/java/org/onosproject/rest/resources/GroupsResourceTest.java
+++ b/web/api/src/test/java/org/onosproject/rest/resources/GroupsResourceTest.java
@@ -276,9 +276,9 @@
         @Override
         public boolean matchesSafely(JsonObject jsonGroup) {
             // check id
-            final String jsonId = jsonGroup.get("id").asString();
+            final Long jsonId = jsonGroup.get("id").asLong();
             final String groupId = group.id().id().toString();
-            if (!jsonId.equals(groupId)) {
+            if (!jsonId.equals(Long.valueOf(groupId))) {
                 reason = "id " + group.id().id().toString();
                 return false;
             }
@@ -362,8 +362,8 @@
                 final JsonObject jsonGroup = json.get(jsonGroupIndex).asObject();
 
                 final String groupId = group.id().id().toString();
-                final String jsonGroupId = jsonGroup.get("id").asString();
-                if (jsonGroupId.equals(groupId)) {
+                final Long jsonGroupId = jsonGroup.get("id").asLong();
+                if (jsonGroupId.equals(Long.valueOf(groupId))) {
                     groupFound = true;
 
                     //  We found the correct group, check attribute values