Handling both SSH & HTTP?

lauyoo46 commented 5 years ago

Hello!

We want to add a functionality where gerrit events plugin retrieves data both through ssh and http.

We have 2 possible solutions for this:

Transform fromJson function in GerritJsonDTO into fromJsonSSH and fromJsonHTTP Modify Account, Approval, Change, Comment, PatchSet, Provider, RefUpdate classes accordingly Transform GerritQueryHandler class to GerritQueryHandlerSSH and add GerritQueryHandlerHTTP class (with a common QueryHandler interface) Adapt the remaining necessary changes

Leave gerrit events as it is and make a converter for http json to ssh json (the json returned by the rest api is very different in structure to the one returned by ssh)

Here is an example of a json returned by ssh

{  
"project":"project",
"branch":"master",
"id":"123123123123",
"number":123,
"subject":"Subject",
"owner":{  
  "name":"name",
  "email":"email",
  "username":"username"
},
"url":"url",
"commitMessage":"commitMessage",
"createdOn":1537137142,
"lastUpdated":1535770916,
"open":true,
"status":"NEW",
"trackingIds":[  
  {  
     "system":"system",
     "id":"id"
  }
],
"currentPatchSet":{  
  "number":11,
  "revision":"revision",
  "parents":[  
     "parents"
  ],
  "ref":"refs",
  "uploader":{  
     "name":"uploader",
     "username":"username"
  },
  "createdOn":1537137142,
  "author":{  
     "name":"name",
     "email":"email",
     "username":"username"
  },
  "kind":"kind",
  "approvals":[  
     {  
        "type":"Validated",
        "description":"Validated",
        "value":"1",
        "grantedOn":1885459715,
        "by":{  
           "name":"name",
           "username":"username"
        }
     },
     {  
        "type":"Code-Review",
        "description":"Code-Review",
        "value":"1",
        "grantedOn":1888644041,
        "by":{  
           "name":"name",
           "username":"username"
        }
     }
  ],
  "sizeInsertions":sizeInsertions,
  "sizeDeletions":-sizeDeletions
    }
}

And here is an example of a json containing the same information, returned by http

{
"id": "id",
"project": "project",
"branch": "master",
"hashtags": [],
"change_id": "change_id",
"subject": "Subject",
"status": "NEW",
"created": "2017-04-06 10:05:42.574000000",
"updated": "2019-02-14 08:21:56.634000000",
"submit_type": "submit_type",
"mergeable": true,
"insertions": insertions,
"deletions": deletions,
"unresolved_comment_count": 0,
"has_review_started": true,
"_number": number,
"owner": {
  "_account_id": _account_id,
  "name": "name",
  "email": "email",
  "username": "username"
},
"labels": {
  "Verified": {
    "all": [
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -1,
          "max": 2
        },
        "_account_id": _account_id,
        "name": "name",
        "username": "username"
      },
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -1,
          "max": 2
        },
        "_account_id": _account_id,
        "name": "name",
        "email": "email",
        "username": "username"
      }
    ],
    "values": {
      "-1": "Fails",
      " 0": "No score",
      "+1": "Formatting is ok",
      "+2": "Build pass with associated unit tests"
    },
    "default_value": 0
  },
  "Code-Review": {
    "all": [
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -2,
          "max": 2
        },
        "_account_id": _account_id,
        "name": "name",
        "username": "username"
      },
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -2,
          "max": 1
        },
        "_account_id": _account_id,
        "name": "name",
        "email": "email",
        "username": "username"
      }
    ],
    "values": {
      "-2": "This shall not be merged",
      "-1": "I would prefer this is not merged as is",
      " 0": "No score",
      "+1": "Looks good to me, but someone else must approve",
      "+2": "Looks good to me, approved"
    },
    "default_value": 0
  },
  "Validated": {
    "all": [
      {
        "value": 0,
        "date": "2017-04-14 08:21:56.634000000",
        "permitted_voting_range": {
          "min": -2,
          "max": 2
        },
        "_account_id": _account_id,
        "name": "name",
        "username": "username"
      },
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -2,
          "max": 1
        },
        "_account_id": _account_id,
        "name": "name",
        "email": "email",
        "username": "username"
      }
    ],
    "values": {
      "-2": "Failed to validate",
      "-1": "Failed to validate tests",
      " 0": "No score",
      "+1": "Passed validation",
      "+2": "Passed validation tests"
    },
    "default_value": 0
  },
  "Priority": {
    "all": [
      {
        "value": 0,
        "permitted_voting_range": {
          "min": -2,
          "max": 1
        },
        "_account_id": _account_id,
        "name": "name",
        "username": "username"
      },
      {
        "value": 0,
        "date": "2017-08-06 10:05:42.574000000",
        "permitted_voting_range": {
          "min": -2,
          "max": 1
        },
        "_account_id": _account_id,
        "name": "name",
        "email": "email",
        "username": "username"
      }
    ],
    "values": {
      "-2": "Blocker",
      "-1": "Critical",
      " 0": "No Priority",
      "+1": "Minor"
    },
    "default_value": 0,
    "optional": true
  }
},
"permitted_labels": {
  "Verified": [
    "-1",
    " 0",
    "+1",
    "+2"
  ],
  "Code-Review": [
    "-2",
    "-1",
    " 0",
    "+1",
    "+2"
  ],
  "Validated": [
    "-2",
    "-1",
    " 0",
    "+1",
    "+2"
  ],
  "Priority": [
    "-2",
    "-1",
    " 0",
    "+1"
  ]
},
"removable_reviewers": [
  {
    "_account_id": _account_id,
    "name": "name",
    "username": "username"
  },
  {
    "_account_id": _account_id,
    "name": "name",
    "email": "email",
    "username": "username"
  }
],
"reviewers": {
  "REVIEWER": [
    {
      "_account_id": _account_id,
      "name": "name",
      "email": "email",
      "username": "username"
    },
    {
      "_account_id": _account_id,
      "name": "name",
      "username": "username"
    }
  ]
},
"pending_reviewers": {},
"current_revision": "current_revision",
"revisions": {
  "revisions": {
    "kind": "NO_CODE_CHANGE",
    "_number": 7,
    "created": "2017-04-08 13:14:33.349000000",
    "uploader": {
      "_account_id": _account_id,
      "name": "name",
      "username": "username"
    },
    "ref": "refs",
    "fetch": {
      "ssh": {
        "url": "ssh://url",
        "ref": "refs"
      },
      "anonymous http": {
        "url": "url",
        "ref": "ref"
      },
      "http": {
        "url": "url",
        "ref": "ref"
      }
    },
    "description": "Edit commit message"
  }
}
}

Let us know what is your feedback on this idea, and which one is more likely to get merged into the repo :)

PS: I saw the issue w/r/t the new architecture proposal, but we don't want to do something that big yet (https://github.com/sonyxperiadev/gerrit-events/issues/10)

rsandell commented 5 years ago

When you say returned by HTTP what do you mean? Is it an HTTP variant of stream-events like a websocket or is it the format of the web hook or something else? Seeing as you are talking about the querry handler I'm guessing the something else :)

I think alternative 1 would be OK. But leave the original methods as is to retain binary backwards compatibility and behaviour.

rsandell commented 5 years ago

The original querry handler should likewise also stay and behave like it used to. Maybe adding a static factory method to create an http or ssh version would work?

sonyxperiadev / gerrit-events

Handling both SSH & HTTP? #88