TurkuForge / baby-project-server

The solution to remote work!
3 stars 1 forks source link

(baby-project-server) #3 initial version of api #4

Closed dusan-turajlic closed 3 years ago

dusan-turajlic commented 3 years ago

Created an initial version of the API. Its currenly very simple, There is a websocket broker and a couple of controllers. You can use a library called SockJS to connect to the webscoket connection point. The root of the API looks like this

{
  "_links": {
    "self": {
      "href": "http://localhost"
    },
    "ws:connect": {
      "href": "http://localhost/connect"
    }
  },
  "_embedded": {
    "channel": [
      {
        "name": "general",
        "subscription": "/channel/general",
        "_links": {
          "self": {
            "href": "http://localhost/channel/general"
          }
        }
      },
      {
        "name": "random",
        "subscription": "/channel/random",
        "_links": {
          "self": {
            "href": "http://localhost/channel/random"
          }
        }
      }
    ]
  }
}

From there you get all the needed information on how to connect to the websocket endoint. The api alos provides channels and all the information you need to sen messages and subscribe to a particular channel, the subscription field is there so that the client can subscribe to a specific channel, we should get use to using that field because in the future it might not be the same as the path.

AndreasNasman commented 3 years ago

Couldn't comment much on the actual Kotlin code since I don't know that language, but a few minor considerations came to mind. 😄

dusan-turajlic commented 3 years ago

@AndreasNasman

Do we want to document every method?

Because this is an opensource project I think we should. We can document stuff in Wikis all we want but code docs is the place most likely to be kept up to date.

Should we have newlines at the end of every file?

We should an this does. if it dident there would be a little red sign at the end of the file complaining that there is no new like at the end of file. Github just does not show the extra line.

image

dusan-turajlic commented 3 years ago

I updated the PR with the config file changes I just need to add some integration tests and we will be good to go. I can the remove the WIP from this PR

dusan-turajlic commented 3 years ago

I removed the WIP status But I will still update the documentasion and generaly take a look at what we want to put where.

dusan-turajlic commented 3 years ago

I have now updated the documentation so this PR is ready for a serious review

edrik commented 3 years ago

Considering what this API endpoint does so far, this might be of interest: https://www.asyncapi.com/

dusan-turajlic commented 3 years ago

Considering what this API endpoint does so far, this might be of interest: https://www.asyncapi.com/

@edrik Just to clarify are you suggesting we are use to https://www.asyncapi.com/ to document our flow? Or am I missing the point of asyncapi?

edrik commented 3 years ago

Considering what this API endpoint does so far, this might be of interest: https://www.asyncapi.com/

@edrik Just to clarify are you suggesting we are use to https://www.asyncapi.com/ to document our flow? Or am I missing the point of asyncapi?

@dusan-turajlic Its a reasonable new endeavor to establish a standardized way to describe a async/pub/sub type API. But it might be a option for describing the websocket parts. Basically linking in the async api description from the api root. But not really suggesting anything specific with the comment other than that it might be worth looking in to.

dusan-turajlic commented 3 years ago

@dusan-turajlic Its a reasonable new endeavor to establish a standardized way to describe a async/pub/sub type API. But it might be a option for describing the websocket parts. Basically linking in the async api description from the api root. But not really suggesting anything specific with the comment other than that it might be worth looking in to.

@edrik Thank you for the suggestion was looking in to it yesturday and it seems really neat. Especialy for a opensource where this info needs to be well documented and easaly available.

dusan-turajlic commented 3 years ago

I updated the code according to the feedback mention by @edrik and @perlun the response now looks like this

{
  "_embedded": {
    "bp:channel": [
      {
        "name": "general",
        "subscription": "/channel/general",
        "_links": {
          "self": {
            "href": "http://localhost:8080/channel/general"
          }
        }
      },
      {
        "name": "random",
        "subscription": "/channel/random",
        "_links": {
          "self": {
            "href": "http://localhost:8080/channel/random"
          }
        }
      }
    ]
  },
  "_links": {
    "bp:sockjs-endpoint": {
      "href": "http://localhost:8080/connect"
    },
    "self": {
      "href": "http://localhost:8080"
    },
    "curies": [
      {
        "href": "https://docs.turkuforge.fi/{#rel}",
        "name": "bp",
        "templated": true
      }
    ]
  }
}

I have no ida why _embedded now comes first but it does not make a difference in paracitce.

I added the DefaultCurieProvider bean and it worked for adding the bp: for any link I add to the resource. But no matter what I tried I could not get it to work when adding embedded content. Migrated to HalModelBuilder to see if that was the problem but no I still needed to manualy specify bp: to channel

edrik commented 3 years ago

I added the DefaultCurieProvider bean and it worked for adding the bp: for any link I add to the resource. But no matter what I tried I could not get it to work when adding embedded content. Migrated to HalModelBuilder to see if that was the problem but no I still needed to manualy specify bp: to channel

That's a bit odd, internaly there is EmbeddedMapper that should pick up the CurieProvider 🤔

dusan-turajlic commented 3 years ago

I'll take another look at it tomorrow maybe I just defined the bean wrong or something 🤷🏻‍♂️

dusan-turajlic commented 3 years ago

@edrik I got it working but I think this might be a bug in the hal model. It now looks like this.

{
  "_embedded": {
    "bp:channelList": [
      {
        "name": "general",
        "subscription": "/channel/general",
        "_links": {
          "self": {
            "href": "http://localhost:8080/channel/general"
          }
        }
      },
      {
        "name": "random",
        "subscription": "/channel/random",
        "_links": {
          "self": {
            "href": "http://localhost:8080/channel/random"
          }
        }
      }
    ]
  },
  "_links": {
    "bp:sockjs-endpoint": {
      "href": "http://localhost:8080/connect"
    },
    "self": {
      "href": "http://localhost:8080"
    },
    "curies": [
      {
        "href": "https://docs.turkuforge.fi/{#rel}",
        "name": "bp",
        "templated": true
      }
    ]
  }
}

Before I specified the LinkRelation like so model.embed( channel, LinkRelation.of("channel") ) that does work becasue it does not take in to considuration the CurieProvider when you specify the realtion as a paramater this might be by design but IDK. I now just removed the the LinkRelation and it now automaticly creates bp:channelList which IMO is a good name for the relation. Might need to dig deeper in to this but its out of the scope of this PR so I will leave it like this for now