Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(api): add api initialization notice #4869

Merged
merged 13 commits into from
Oct 29, 2024

Conversation

gacevicljubisa
Copy link
Contributor

@gacevicljubisa gacevicljubisa commented Oct 19, 2024

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

This PR addresses issue #4734 by improving the behavior of the Bee API during the node initialization process. Previously, when the node was syncing batch data, the API appeared to be inaccessible, providing no indication of the ongoing sync process. Now, all routes are mounted immediately, but some may be temporarily disabled during initialization, returning appropriate status codes.

Changes

  • 503 Service Unavailable: Routes that are disabled during the syncing phase now return a 503 Service Unavailable status with the message:
    "Node is syncing. This endpoint is unavailable. Try again later."
    Once the syncing is complete, the routes will be enabled and respond normally.

  • 501 Not Implemented: If the swapEnabled or chequebookEnabled flags are set to false, routes that depend on these features will now return a 501 Not Implemented status.

  • 404 Not Found: Any routes that do not exist will return a 404 Not Found status.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

This change improves the user experience by providing clear feedback during node initialization, preventing confusion when the API appears to be inaccessible. It ensures that users are informed about the node's state and can understand why certain API routes may not be available temporarily.

How it works

  • All routes are mounted immediately but may not be active depending on the node's state.
  • Disabled routes return 503 until they are enabled once the sync is complete.
  • Feature-dependent routes (like those for Swap and Chequebook) return 501 if the corresponding feature is disabled.
  • Invalid or non-existent routes return a 404 as expected.

Related Issue (Optional)

#4734

Screenshots (if appropriate):

@gacevicljubisa gacevicljubisa added the in progress ongoing development , hold out with review label Oct 19, 2024
s.router = mux.NewRouter()
s.router.NotFoundHandler = http.HandlerFunc(jsonhttp.NotFoundHandler)
// WithFullAPI will enable all available endpoints, because some endpoints are not available during syncing.
func WithFullAPI() MountOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have an option if we can explicitly enable the the full API using EnableFullAPI()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial idea to provide options is for unit testing purposes. Was considering to add WithFullAPIDisabled option as well. And then expand testServerOptions in api_test.go.
Also it can be used in one line to mount and enable apiService.Mount(api.WithFullAPI()), which is the case in devnode.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option is removed

@gacevicljubisa gacevicljubisa removed the in progress ongoing development , hold out with review label Oct 22, 2024
@gacevicljubisa gacevicljubisa linked an issue Oct 22, 2024 that may be closed by this pull request
@gacevicljubisa gacevicljubisa marked this pull request as ready for review October 22, 2024 15:53
@istae istae changed the title fix(api): add api initialization notice v2 fix(api): add api initialization notice Oct 23, 2024
Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@istae istae merged commit f5ba590 into master Oct 29, 2024
14 checks passed
@istae istae deleted the fix/add-api-initialization-notice_v2 branch October 29, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add notice that API is not accessible during initialization
4 participants