xbrowsersync / api

Server-side REST API that serves requests from xBrowserSync client apps
GNU General Public License v3.0
796 stars 83 forks source link

Need help, I'm trying to port API code to .NET Core #12

Closed hadi-susanto closed 5 years ago

hadi-susanto commented 6 years ago

Hi @nero120,

Currently I'm challenging myself to create similar API server using .NET Core, I already able to handle create, update, info sync request successfully but I unable to handle invalid bookmark ID correctly.

For now when xBrowserSync request for non-existing ID i would give HTTP 404 response with empty body and xBrowserSync assume that connection to server is lost as follow

2018-06-30 21_30_18

Also I got confused about the response when sync size limit is exceeded or not accepting new sync? Based on your bookmarksService.ts it will throw an exception but I couldn't understand what happen next? How the exception is handled and translated into JSON response? Could you tell me the expected JSON response for xBrowserSync?

Lastly I would ask that would you allow http://localhost as API server address for xBrowserSync? It seems have validation that reject http://localhost

Thank You

nero120 commented 6 years ago

For now when xBrowserSync request for non-existing ID i would give HTTP 404 response with empty body and xBrowserSync assume that connection to server is lost as follow

Incorrect handling non-existing ids was a bug I fixed recently in v1.1.3. If an id is provided that does not exist, an InvalidSyncIdException is thrown which ultimately returns a 409 HTTP status code. The latest app version will interpret this properly and display the correct error message to the user.

Also I got confused about the response when sync size limit is exceeded or not accepting new sync? Based on your bookmarksService.ts it will throw an exception but I couldn't understand what happen next? How the exception is handled and translated into JSON response? Could you tell me the expected JSON response for xBrowserSync?

Hmm, from the reference to bookmarksService.ts it looks like you are not using the latest version of v1.1.3, I recommend you update your dotnet core code to use the latest version as a number of fixes have been committed and updates so that the API works with the latest app version due to be released soon. But to answer your question, exceptions are handled in the handleErrors method in server.ts, which maps the generated exception to an HTTP status code via the ExceptionBase base class and child classes listed in exception.ts. You can see what the correct HTTP status code for each exception is in there.

Lastly I would ask that would you allow http://localhost as API server address for xBrowserSync? It seems have validation that reject http://localhost

This will be resolved in the upcoming v1.4.0 of the app. In the meantime you can create a local hostname to get around this restriction.

hadi-susanto commented 6 years ago

Hi, it seems that I was using older (1.1.2) base code since when I start I still use from master branch and don't use v1.1.3 branch

Thank you for explanation I will try to match mine to your. Also I see a new createBookmark_v2 which mapped to version 1.1.3 and I quite confused how the endpoint is mapped in this block

2018-07-06 21_52_21-d__downloads_api-master_src_routers_bookmarks router ts - notepad

Does the create bookmarks endpoint now become /booksmarks/1.0.0 and /bookmarks/1.1.3 ? I have zero knowledge regarding 'express-routes-versioning'. Thank you for your assistance

EDIT: after some reading 'express-routes-versioning' use HTTP Header 'Accept-Version' is that right? If yes could you tell me the symbol meaning such as ^ and ~

nero120 commented 6 years ago

@hadirena sorry for not getting back to you sooner, I've been distracted by work on v1.4.0 of the client. You are correct when you say it looks at the Accept-Version HTTP header to determine the requested API version. The characters in the version numbers are a feature of Semantic Versioning the same as NPM packages:

So in the case of create Bookmarks, the endpoint is always /bookmarks, but the service method used will be determined by the value supplied in the Accept-Version header (if this header is not supplied or empty, the current API version will be used).

FYI, I've just committed fa9ea3d which makes a slight change to the bookmarks routes (^ should always be used for latest route so that if minor version is updated the route will still apply), and also removes the config/version.json file as it's redundant since the value is identical to the version value in package.json and it saves having to update the version in multiple places.

Metal-Frog commented 6 years ago

@hadirena Would you share your code?

hadi-susanto commented 6 years ago

@hadirena Would you share your code?

Hmm i don't mind to share, but it I haven't got time to update it with latest server changes, it outdated. My code still based on 1.1.3 code. is it ok?

BrutalSimplicity commented 5 years ago

I wouldn’t mind helping with this. I guess I’d first like to know why you’re porting this code to .Net Core. Is there some value to doing that?

hadi-susanto commented 5 years ago

Hi @BrutalSimplicity ,

I started this port purely just for fun since at that time I just learn .net core and want to practice. I found this project would be suitable for my learning. TBH I just use this for my personal need

I just code up to ver 1.1.3 and didn't follow up the latest changes yet. Anyway if @nero120 allow and want .net core port to be published I would be happy to support and welcome any help.

nero120 commented 5 years ago

In my honest opinion, since implementing the API service docker image, I can't see a great deal of value in porting the code to a different platform, especially since I tend to release fairly regularly. Docker is supported on every platform now so there's no barrier to getting a personal xBrowserSync service running whichever platform you have.

I appreciate the offer from both of you @hadirena and @BrutalSimplicity, but on reflection I don't think it's something I'd offer here, however by all means continue porting for your own needs/requirements! @hadirena maybe share a link to your repo here so others can find it? 💡

In light of this, I'll close this request, but feel free to continue the discussion here if required.