Basic service scaffolding using Scala Play framework #18
Conversation
WalkthroughThe changes introduce a new Play Framework web service project, including updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FrontendController
participant View
User->>FrontendController: GET /
FrontendController->>View: Render index with message
View-->>FrontendController: Return HTML
FrontendController-->>User: Display homepage
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (10)
webservice/conf/routes (2)
3-4: LGTM: Static assets route is correctly defined.The route for static assets is properly configured, using the
controllers.Assets.versionedmethod to serve files from the "/public" folder. This follows Play Framework best practices for efficient management of static resources.Consider enhancing the comment to provide more specific information:
- # Map static resources from the /public folder to the /assets URL path + # Serve static resources (e.g., images, CSS, JavaScript) from the /public folder via the /assets URL pathThis minor change adds clarity about the types of resources typically served through this route.
1-4: Consider future route additions.The current routes file provides a solid foundation for a basic web application. However, based on the PR objectives, there are plans to add more controllers and endpoints. As you proceed with development, remember to:
- Add routes for the Backend controller for metrics data retrieval.
- Consider organizing routes logically as the application grows, possibly grouping them by functionality or controller.
- Ensure that new routes follow the same clear naming and commenting conventions established in this initial setup.
webservice/app/views/index.scala.html (3)
3-8: Enhance the head section for better HTML5 compliance and responsivenessThe basic structure is good, but consider adding the following to improve the template:
- Add a meta charset declaration for proper character encoding.
- Include a viewport meta tag for better responsive design support.
Here's a suggested improvement:
<!DOCTYPE html> <html lang="en"> <head> + <meta charset="utf-8"> + <meta name="viewport" content="width=device-width, initial-scale=1"> <title>Chronon Home</title> <link rel="shortcut icon" type="image/png" href="@routes.Assets.versioned("images/favicon.png")"> </head>
9-11: LGTM: Body content is correct, with room for future expansionThe body content correctly renders the
@messageparameter within an<h1>tag. This is suitable for a basic "hello world" page.As the project evolves, consider:
- Adding more semantic HTML5 elements (e.g.,
<header>,<main>,<footer>) for better structure.- Including placeholder elements for future content sections.
- Preparing for the integration of Svelte components, as mentioned in the PR objectives.
1-12: Consider future scalability and maintainabilityThe template provides a good starting point, but as the application grows, consider the following improvements:
- Create a separate layout template that this and other pages can extend. This promotes code reuse and consistency across pages.
- Prepare for including CSS and JavaScript files, possibly using the
@routes.Assets.versioned()helper for cache busting.- Add comments to explain any complex logic or non-obvious uses of the template syntax.
Here's an example of how you might structure a layout template in the future:
@(title: String)(content: Html) <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <meta name="viewport" content="width=device-width, initial-scale=1"> <title>@title</title> <link rel="shortcut icon" type="image/png" href="@routes.Assets.versioned("images/favicon.png")"> <link rel="stylesheet" href="@routes.Assets.versioned("stylesheets/main.css")"> </head> <body> @content <script src="@routes.Assets.versioned("javascripts/main.js")"></script> </body> </html>Then, your
index.scala.htmlcould be simplified to:@(message: String) @main("Chronon Home") { <h1>@message</h1> }This structure will make it easier to maintain consistent layouts across multiple pages as your application grows.
webservice/app/controllers/FrontendController.scala (1)
6-7: LGTM: Class definition follows Play Framework best practices.The
FrontendControllerclass is well-structured:
- It's correctly annotated as a
@Singleton.- It extends
BaseControlleras recommended.- It uses constructor injection for
ControllerComponents.Consider adding a brief comment describing the purpose of this controller for improved clarity:
@Singleton +/** Controller for handling frontend-related requests */ class FrontendController @Inject()(val controllerComponents: ControllerComponents) extends BaseController {webservice/conf/application.conf (4)
6-6: Consider future internationalization needsThe current configuration sets English as the only supported language, which is fine for now.
If there are plans for internationalization in the future, consider:
- Adding a comment about potential expansion.
- Researching Play's i18n capabilities for smooth transition later.
8-14: Prepare for database integrationThe commented database configuration provides a good starting point for future database setup.
Next steps to consider:
- Decide on the production database (e.g., PostgreSQL, MySQL) and add appropriate configuration.
- Use environment variables for database credentials:
db.default.driver=${?DB_DRIVER} db.default.url=${?DB_URL} db.default.username=${?DB_USER} db.default.password=${?DB_PASSWORD}- Provide clear documentation on setting up the development database.
16-22: Make an explicit decision about database evolutionsThe current configuration leaves the decision about database evolutions ambiguous.
Consider:
- Making an explicit decision about whether to use evolutions.
- If using evolutions, uncomment and set
play.evolutions.enabled=true.- If not using evolutions, you may want to explicitly disable them:
play.evolutions.enabled=false- Document the decision and its implications in a comment.
1-22: Enhance overall configuration structure and documentationThe configuration file provides a good foundation, but there's room for improvement in structure and documentation.
Consider the following enhancements:
- Add more comprehensive comments explaining each section and its importance.
- Group related configurations (e.g., all database-related configs together).
- Include placeholders for other common Play Framework configurations (e.g., akka settings, custom application configs).
- Add a section for environment-specific overrides and document how to use them.
- Consider using HOCON syntax for more advanced configuration structures.
- Add references to Play Framework documentation for each major section.
Example structure:
# Application Configuration # ~~~~~ # Secret key # ~~~~~ play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY} # Internationalization # ~~~~~ play.i18n.langs = ["en"] # Database Configuration # ~~~~~ # db.default { # driver = ${?DB_DRIVER} # url = ${?DB_URL} # username = ${?DB_USER} # password = ${?DB_PASSWORD} # } # Evolutions # ~~~~~ play.evolutions.enabled = false # Akka Configuration # ~~~~~ # akka { # actor { # default-dispatcher { # fork-join-executor { # parallelism-factor = 1.0 # parallelism-max = 24 # } # } # } # } # Custom Application Config # ~~~~~ # myapp { # feature.x.enabled = true # feature.y.url = "https://example.com" # } # Include environment-specific config include "application.${PLAY_ENV}.conf"
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
webservice/public/images/favicon.pngis excluded by!**/*.png
Files selected for processing (6)
- build.sbt (3 hunks)
- project/plugins.sbt (1 hunks)
- webservice/app/controllers/FrontendController.scala (1 hunks)
- webservice/app/views/index.scala.html (1 hunks)
- webservice/conf/application.conf (1 hunks)
- webservice/conf/routes (1 hunks)
Additional comments not posted (8)
webservice/conf/routes (1)
1-1: LGTM: Home page route is correctly defined.The route for the home page is properly configured, mapping the root URL to the
home()method ofFrontendController. This follows Play Framework best practices for defining the main entry point of the application.webservice/app/views/index.scala.html (1)
1-1: LGTM: Template parameter is well-definedThe template parameter
message: Stringis correctly defined and follows good practices for simple templates.webservice/app/controllers/FrontendController.scala (1)
1-5: LGTM: Package and imports are correct.The package declaration and imports are appropriate for a Play Framework controller. They include the necessary components for dependency injection and MVC functionality.
project/plugins.sbt (1)
10-10: LGTM! Verify Play Framework version compatibility.The addition of the Play Framework SBT plugin (version 3.0.5) is correct and aligns with the PR objectives. This will enable Play Framework specific tasks and configurations in your SBT build.
Please ensure that this version of the Play Framework plugin is compatible with your Scala version and other project dependencies. Run the following command to check for any version conflicts:
build.sbt (4)
20-20: LGTM: Addition of Scala 2.13 versionThe addition of Scala 2.13.14 is appropriate for the Play framework, which requires Scala 2.13. This version is also the latest stable release of Scala 2.13 as of September 2024.
53-53: LGTM: Addition of webservice to root project aggregationThe inclusion of
webservicein the root project aggregation is correct and aligns with the PR objectives to introduce a new web service using the Play framework.
Line range hint
194-202: Verify assembly merge strategy for Play framework compatibilityWhile the assembly merge strategy remains unchanged, it's important to ensure it's still appropriate with the addition of the new
webservicemodule using the Play framework. Consider reviewing and potentially adjusting the strategy to handle any Play framework-specific files or conflicts that may arise during assembly.#!/bin/bash # List all files in the Play framework JAR to identify potential conflicts play_jar=$(find ~/.ivy2/cache -name "play-server_2.13-*.jar" | head -n 1) if [ -n "$play_jar" ]; then echo "Files in Play framework JAR:" jar tf "$play_jar" | grep -E "META-INF/|reference.conf|play/|application.conf" else echo "Play framework JAR not found. Make sure it's downloaded in your ivy2 cache." fi
182-192: LGTM: Webservice project definitionThe
webserviceproject definition is well-structured and aligns with the PR objectives:
- Correctly enables the PlayScala plugin
- Uses Scala 2.13 as required by Play framework
- Includes Guice for dependency injection
- Adds ScalaTestPlus Play for testing
However, consider verifying if version 5.1.0 of ScalaTestPlus Play is the latest available version. Updating to the most recent version could provide bug fixes and new features.
| def home(): Action[AnyContent] = Action { implicit request: Request[AnyContent] => | ||
| Ok(views.html.index("Welcome to the Zipline homepage!")) | ||
| } |
There was a problem hiding this comment.
LGTM with suggestions: Consider internationalization and type safety improvements.
The home method is correctly implemented as a Play Framework action. It properly renders a view with a welcome message.
Consider the following improvements:
- Internationalization: Instead of hardcoding the welcome message, use Play's i18n support:
- Ok(views.html.index("Welcome to the Zipline homepage!"))
+ Ok(views.html.index(Messages("home.welcome")))Don't forget to add the corresponding message in your conf/messages file:
home.welcome=Welcome to the Zipline homepage!
- Type safety: Use Play's reverse routing for better type safety and maintainability:
- def home(): Action[AnyContent] = Action { implicit request: Request[AnyContent] =>
+ def home: Action[AnyContent] = Action { implicit request: Request[AnyContent] =>
Ok(views.html.index(Messages("home.welcome")))
}This change allows you to reference the action as routes.FrontendController.home in your routes file and views, providing compile-time checking of your routes.
Committable suggestion was skipped due to low confidence.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
As discussed, we will change a few things in a future PR but I am happy with using play to serve the frontend code
build.sbt
Outdated
|
|
||
| lazy val root = (project in file(".")) | ||
| .aggregate(api, aggregator, online, spark, flink, cloud_gcp) | ||
| .aggregate(api, aggregator, online, spark, flink, cloud_gcp, webservice) |
There was a problem hiding this comment.
nit: rename to "hub"? That is likely also the name we will use when we market to end-users
hub is a place for job tracking, monitoring - data and system quality, fetcher playground, authoring etc.
I am imagining this folder will evolve to encompass all of that.
There was a problem hiding this comment.
Yeah I like hub. Lets go with that
| // java incompatibility is probably not an issue, hopefully we can cross build flink 1.17 & 1.18 without code changes | ||
|
|
||
| lazy val scala_2_12 = "2.12.18" | ||
| lazy val scala_2_13 = "2.13.14" |
There was a problem hiding this comment.
I like the simplicity of one scala version. But if it is not possible to be 2.12 with play, this is workable for me.
There was a problem hiding this comment.
Yeah 2.12 is not possible with play unless we go with a much older release. 2.9.x that is on the akka fork of the project. I think it might be better to use play 3.x and hence scala 2.13 here.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
hub/conf/routes (1)
1-4: Consider adding more routes to fulfill PR objectives.While the existing routes are correctly defined, the routes file seems incomplete given the PR objectives:
- There are no routes defined for the Backend controllers mentioned in the PR objectives for metrics data retrieval.
- Error handling routes (e.g., for 404 Not Found) are missing, which could lead to poor user experience.
Consider adding these routes to fully implement the intended functionality of the web service.
hub/app/views/index.scala.html (2)
5-8: Add meta charset declaration for proper character encoding.The head section looks good with a descriptive title and favicon. However, it's recommended to add a meta charset declaration for proper character encoding.
Consider adding the following line just after the opening
<head>tag:<meta charset="utf-8">This ensures that the browser interprets the page content correctly.
9-11: LGTM: Body section correctly implements dynamic content.The body section correctly implements the dynamic message using Scala template syntax.
For future development, consider the following suggestions:
- Add more semantic HTML structure (e.g.,
<header>,<main>,<footer>) as the page content grows.- Include placeholder elements or comments for upcoming features mentioned in the PR objectives, such as controllers for Frontend and Backend, and scaffolding for Svelte and ECharts modules.
- Consider adding CSS classes or IDs to facilitate styling and JavaScript interactions in the future.
hub/app/controllers/FrontendController.scala (2)
7-8: LGTM: Class definition follows Play framework best practices.The
FrontendControllerclass is correctly defined as a singleton with proper dependency injection. It extendsBaseControlleras expected.Consider adding a brief class-level documentation comment to describe the purpose of this controller:
/** * Controller for handling frontend requests. */ @Singleton class FrontendController @Inject() (val controllerComponents: ControllerComponents) extends BaseController { // ... }
9-12: LGTM with suggestions: Consider internationalization and type safety improvements.The
homemethod is correctly implemented as a Play framework action. However, there are a couple of areas for potential improvement:
Internationalization: The welcome message is hardcoded. Consider using Play's internationalization features for better maintainability and localization support.
Type safety: The
indexview is called with aStringparameter, but it's not clear from this code what type the view expects. Consider using a type-safe view model.Here's a suggested refactoring to address these points:
import play.api.i18n.{I18nSupport, MessagesApi} class FrontendController @Inject() ( val controllerComponents: ControllerComponents, val messagesApi: MessagesApi ) extends BaseController with I18nSupport { def home(): Action[AnyContent] = Action { implicit request => Ok(views.html.index(HomeViewModel(messagesApi("welcome.message")))) } } case class HomeViewModel(welcomeMessage: String)This refactoring:
- Uses
MessagesApifor internationalization.- Introduces a
HomeViewModelfor type-safe view rendering.Don't forget to add the corresponding message in your
conf/messagesfile:welcome.message=Welcome to the Zipline homepage!hub/conf/application.conf (1)
5-5: LGTM. Consider future internationalization needs.The current configuration for supported languages is appropriate for an initial setup. If there are plans to support multiple languages in the future, you may want to add a comment indicating this, such as:
play.i18n.langs = ["en"] +# TODO: Add support for additional languages as neededThis will serve as a reminder for future development.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
hub/public/images/favicon.pngis excluded by!**/*.png
Files selected for processing (5)
- build.sbt (5 hunks)
- hub/app/controllers/FrontendController.scala (1 hunks)
- hub/app/views/index.scala.html (1 hunks)
- hub/conf/application.conf (1 hunks)
- hub/conf/routes (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- build.sbt
Additional comments not posted (7)
hub/conf/routes (2)
1-1: LGTM: Home page route is correctly defined.The route for the home page is properly configured. It maps the root URL ("/") to the
home()method ofFrontendControllerusing the GET HTTP method, which is the standard approach for serving a home page.
3-4: LGTM: Static assets route is correctly defined.The route for serving static assets is properly configured. It uses the
Assets.versionedmethod, which is good for cache busting. The comment above the route provides clear explanation of its purpose.hub/app/views/index.scala.html (2)
1-1: LGTM: Template parameter correctly defined.The template parameter
@(message: String)is properly defined, allowing for dynamic content to be passed to the template.
3-4: LGTM: Proper HTML structure and language declaration.The HTML structure is well-formed with the correct doctype declaration. The
langattribute is properly set to "en", which is good for accessibility and SEO.Also applies to: 12-12
hub/app/controllers/FrontendController.scala (1)
1-5: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct for a Play framework controller. The code imports only the necessary components for MVC and dependency injection.
hub/conf/application.conf (2)
15-21: Clarify the intended use of evolutions.The evolutions configuration is currently commented out. It's important to clarify whether database evolutions are intended to be used in this project. Consider:
- If evolutions are not needed, add a comment explaining why:
# Evolutions # ~~~~~ -# You can disable evolutions if needed +# Evolutions are not used in this project because [reason] # play.evolutions.enabled=false # You can disable evolutions for a specific datasource if necessary # play.evolutions.db.default.enabled=false
- If evolutions will be used, uncomment and set the appropriate values:
# Evolutions # ~~~~~ -# You can disable evolutions if needed -# play.evolutions.enabled=false +# Enable evolutions for automatic database migrations +play.evolutions.enabled=true # You can disable evolutions for a specific datasource if necessary -# play.evolutions.db.default.enabled=false +play.evolutions.db.default.enabled=trueThis will help maintain clarity about the project's database migration strategy.
To check if there are any evolution scripts in the project, you can run:
#!/bin/bash # Search for evolution scripts fd -e sql -p 'conf/evolutions'
7-13: Clarify the database configuration.The database configuration is currently commented out. While using an in-memory H2 database is common for development and testing, it's important to clarify the intended database setup. Consider either:
- Uncommenting the configuration if it represents the intended setup:
-# db.default.driver=org.h2.Driver -# db.default.url="jdbc:h2:mem:play" +db.default.driver=org.h2.Driver +db.default.url="jdbc:h2:mem:play"
- Or, if a different database is being used, add a comment explaining the current setup:
+# Current database configuration is managed externally +# TODO: Add production database configuration here # db.default.driver=org.h2.Driver # db.default.url="jdbc:h2:mem:play"This will help other developers understand the database configuration at a glance.
To check if there's any database configuration elsewhere in the project, you can run:
Verification successful
Clarify the database configuration.
The database configuration is currently commented out. While using an in-memory H2 database is common for development and testing, it's important to clarify the intended database setup. Consider either:
Uncommenting the configuration if it represents the intended setup:
-# db.default.driver=org.h2.Driver -# db.default.url="jdbc:h2:mem:play" +db.default.driver=org.h2.Driver +db.default.url="jdbc:h2:mem:play"Adding a comment to explain the current setup if a different database is being used:
+# Current database configuration is managed externally +# TODO: Add production database configuration here # db.default.driver=org.h2.Driver # db.default.url="jdbc:h2:mem:play"This will help other developers understand the database configuration at a glance.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for database configuration in other files rg --type-add 'conf:*.{conf,properties}' --type conf 'db\.default\.(driver|url)'Length of output: 173
Script:
#!/bin/bash # Corrected script to search for database configuration in .conf and .properties files rg 'db\.default\.(driver|url)' --type-add 'conf:*.{conf,properties}' --type confLength of output: 204
| # This is the main configuration file for the application. | ||
| # https://www.playframework.com/documentation/latest/ConfigFile | ||
|
|
||
| play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a fallback value for the HTTP secret key.
While using an environment variable for the secret key is a good security practice, it's important to ensure the application can start even if the environment variable is not set. Consider adding a fallback value:
-play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY}
+play.http.secret.key = "changeme" # Default fallback value
+play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY}Remember to change the default value in production. The secret key is crucial for encrypting session data and other security-related tasks.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY} | |
| play.http.secret.key = "changeme" # Default fallback value | |
| play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY} |
Basic service scaffolding using Scala Play framework
Basic service scaffolding using Scala Play framework
Basic service scaffolding using Scala Play framework
Summary
First stab at spinning up a webservice framework using Play. Did explore a couple of other webframeworks as part of this before settling on play:
PR changes itself are fairly minimal. Spins up a small hello world webservice. If we're peaceful with the choice, we can proceed with adding two sets of controllers - Frontend (for the endpoints we want to expose to the users from the website) and Backend (for the endpoints that the frontend will hit to pull up metrics data etc). We can also start setting up the scaffolding for our svelte / echarts modules etc.
Pulling up the web UI in Chrome:
Checklist
Summary by CodeRabbit
New Features
FrontendControllerfor both thewebserviceandhubprojects, featuring a homepage that displays a welcome message.Configuration
Dependencies