Skip to content

Implement stubbed backend APIs for model list, search and time series #22

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

Merged
merged 14 commits into from
Sep 30, 2024

Conversation

piyush-zlai
Copy link
Contributor

@piyush-zlai piyush-zlai commented Sep 27, 2024

Summary

Add some stubbed APIs based on the outline sketched out in doc. As part of this I've added some basic validation of the endpoint params. We'll need to do a more thorough pass there and lock things down so that the backend APIs can only be hit by the frontend in a subsequent PR.

Checklist

  • Added Unit Tests
  • Covered by existing CI
  • Integration tested
  • Documentation update

Test by unit tests + manual. For manual testing and to see the output:

sbt "project hub" run

Then in a browser / via curl, you can pull up some of these endpoints to see the returned responses:

http://localhost:9000/api/v1/models
http://localhost:9000/api/v1/search?term=1&limit=20


http://localhost:9000/api/v1/model/1/timeseries?startTs=1725926400000&endTs=1726106400000&offset=10h&algorithm=psi
http://localhost:9000/api/v1/model/1/timeseries/slice/123?startTs=1725926400000&endTs=1726106400000&offset=10h&algorithm=psi

http://localhost:9000/api/v1/join/my_join/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=drift&metrics=null&offset=10h&algorithm=psi
http://localhost:9000/api/v1/join/my_join/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=skew&metrics=null

http://localhost:9000/api/v1/join/my_join/timeseries/slice/123?startTs=1725926400000&endTs=1726106400000&metricType=drift&metrics=null&offset=10h&algorithm=psi
http://localhost:9000/api/v1/join/my_join/timeseries/slice/123?startTs=1725926400000&endTs=1726106400000&metricType=skew&metrics=null

http://localhost:9000/api/v1/feature/my_feat/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=drift&metrics=null&offset=10h&algorithm=psi&granularity=aggregates
http://localhost:9000/api/v1/feature/my_feat/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=drift&metrics=null&offset=10h&algorithm=psi&granularity=percentile

http://localhost:9000/api/v1/feature/my_feat/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=skew&metrics=null&granularity=raw
http://localhost:9000/api/v1/feature/my_feat/timeseries?startTs=1725926400000&endTs=1726106400000&metricType=skew&metrics=null&granularity=percentile

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced ApplicationController, ModelController, SearchController, and TimeSeriesController for handling various API requests, including endpoints for listing models, searching, and retrieving time series metrics.
    • Added a health check endpoint at /api/v1/ping.
  • Improvements

    • Updated library dependencies for enhanced JSON handling and testing capabilities, including the addition of the Circe library.
  • Bug Fixes

    • Implemented error handling for invalid pagination parameters across various endpoints.
  • Tests

    • Added unit tests for ModelController, SearchController, and TimeSeriesController to ensure proper functionality and error handling.

Copy link

coderabbitai bot commented Sep 27, 2024

Walkthrough

The pull request introduces several new components to the application, including multiple controller classes for handling various API endpoints related to models and time series data. It updates the project's build configuration to include new library dependencies for JSON handling with Circe and modifies testing frameworks. Additionally, a new configuration file is added to manage application settings, and unit tests are introduced for the new controllers to ensure proper functionality.

Changes

Files Change Summary
build.sbt Introduced circeVersion variable and added Circe library dependencies. Updated scalatestplus-play library version.
hub/app/controllers/ApplicationController.scala Added ApplicationController class with a ping method for health checks.
hub/app/controllers/ModelController.scala Introduced ModelController class with a list method for paginated model listings.
hub/app/controllers/SearchController.scala Added SearchController class with a search method for model searching, including pagination and mock data generation.
hub/app/controllers/TimeSeriesController.scala Introduced TimeSeriesController class with multiple methods for fetching time series data related to models, joins, and features.
hub/app/model/Model.scala Added Model case class and various sealed traits for metrics, algorithms, and time series data structures.
hub/conf/application.test.conf New configuration file for application settings, including HTTP secret key and database configuration.
hub/conf/routes Added new API endpoints for ping, model listing, searching, and time series data retrieval.
hub/test/controllers/ModelControllerSpec.scala Introduced unit tests for ModelController to validate the list method functionality.
hub/test/controllers/SearchControllerSpec.scala Added unit tests for SearchController to ensure proper search functionality and error handling.
hub/test/controllers/TimeSeriesControllerSpec.scala Created unit tests for TimeSeriesController to validate time series data retrieval methods.

Possibly related PRs

  • Basic service scaffolding using Scala Play framework #18: The changes in build.sbt involve modifications to library dependencies and project structure, which are related to the updates made in the main PR regarding the addition of new library dependencies for Circe and the update of the scalatestplus-play library.

Suggested reviewers

  • ken-zlai
  • nikhil-zlai

Poem

In the meadow where rabbits play,
New controllers hop in, bright as day.
With Circe's charm, they fetch and serve,
Models and metrics, they deftly curve.
A ping for health, a search so spry,
In our code garden, watch it fly! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bdb5136 and fbd16d9.

📒 Files selected for processing (1)
  • hub/app/controllers/TimeSeriesController.scala (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • hub/app/controllers/TimeSeriesController.scala

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 11

🧹 Outside diff range and nitpick comments (18)
hub/app/controllers/ApplicationController.scala (2)

1-5: Consider using more specific imports.

While the current imports are valid, consider replacing the wildcard import play.api.mvc._ with more specific imports for better code clarity and to avoid potential naming conflicts. For example:

import play.api.mvc.{Action, AnyContent, BaseController, ControllerComponents, Request}

This approach makes it clearer which components from the mvc package are being used in the controller.


11-14: LGTM: Simple and effective ping implementation.

The ping() method is correctly implemented:

  • It returns an Action[AnyContent] as expected for a Play controller method.
  • The response is a simple "pong!" string, which is appropriate for a basic health check.

Consider adding a content type to the response for better HTTP compliance:

Ok("pong!").as("text/plain")

This explicitly sets the content type of the response to plain text.

hub/conf/application.test.conf (3)

4-4: LGTM! Consider adding a fallback for the secret key.

Using an environment variable for the secret key is a good security practice. However, to prevent potential issues if the environment variable is not set, consider adding a fallback value for development purposes.

You could modify the line as follows:

-play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY}
+play.http.secret.key = "changeme" # Default for development
+play.http.secret.key = ${?PLAY_HTTP_SECRET_KEY}

This ensures a default value for development while still allowing it to be overridden by the environment variable in production.


5-5: LGTM! Consider future multi-language support.

The configuration for internationalization is clear and straightforward. However, as the application grows, you might want to consider supporting multiple languages to improve accessibility.

For future development, you could expand this configuration to include more languages:

play.i18n.langs = ["en", "es", "fr", "de"]

This would allow for easier integration of translations as your user base grows.


15-21: LGTM! Consider adding a comment about the current evolutions state.

The commented-out configurations for disabling evolutions are clear and provide good flexibility. However, it might be helpful to add a comment explicitly stating that evolutions are currently enabled by default.

Consider adding a comment at the beginning of this section to clarify the current state:

# Evolutions
# ~~~~~
# Evolutions are currently enabled by default.
# You can disable evolutions if needed
# play.evolutions.enabled=false

# You can disable evolutions for a specific datasource if necessary
# play.evolutions.db.default.enabled=false

This addition would make the current configuration state more explicit for other developers.

hub/app/controllers/ModelController.scala (2)

16-26: LGTM: Mock data generation is well-implemented, but consider adding a TODO for future replacement.

The mock data generation is clearly marked as temporary and provides a good variety of models for testing. The use of private methods and fields is appropriate for encapsulation.

Consider adding a TODO comment to remind about replacing the mock data with actual data from the KV store layer in the future. For example:

 // temporarily serve up mock data while we wait on hooking up our KV store layer
+// TODO: Replace mock data with actual data from KV store layer
 private[this] def generateMockModel(id: String): Model =

37-51: LGTM with suggestions: Implementation is solid, but could be improved.

The implementation correctly handles pagination, performs basic input validation, and returns the data in the expected format. However, there are a few areas that could be improved:

  1. Input validation could be more comprehensive. Consider adding an upper limit for the limit parameter to prevent potential performance issues with very large requests.

  2. There's no explicit handling of the case where the offset is beyond the available data. In this case, an empty list will be returned, which might be confusing for API consumers.

  3. The error messages for invalid inputs could be more informative, possibly including the actual value that caused the error.

Consider applying the following improvements:

 def list(offset: Option[Int], limit: Option[Int]): Action[AnyContent] =
   Action { implicit request: Request[AnyContent] =>
     // Default values if the parameters are not provided
     val offsetValue = offset.getOrElse(defaultOffset)
     val limitValue = limit.getOrElse(defaultLimit)
+    val maxLimit = 100 // Define a maximum limit to prevent performance issues
 
     if (offsetValue < 0) {
-      BadRequest("Invalid offset - expect a positive number")
+      BadRequest(s"Invalid offset: $offsetValue. Expected a non-negative number.")
     } else if (limitValue < 0) {
-      BadRequest("Invalid limit - expect a positive number")
+      BadRequest(s"Invalid limit: $limitValue. Expected a positive number.")
+    } else if (limitValue > maxLimit) {
+      BadRequest(s"Invalid limit: $limitValue. Maximum allowed limit is $maxLimit.")
     } else {
       val paginatedResults = mockModelRegistry.slice(offsetValue, offsetValue + limitValue)
+      if (paginatedResults.isEmpty && offsetValue > 0) {
+        NotFound(s"No results found for offset: $offsetValue")
+      } else {
         val json = ListModelResponse(offsetValue, paginatedResults).asJson.noSpaces
         Ok(json)
+      }
     }
   }

These changes improve input validation, add handling for out-of-range offsets, and provide more informative error messages.

hub/test/controllers/ModelControllerSpec.scala (4)

1-20: LGTM! Consider organizing imports for better readability.

The imports and class declaration look good. The necessary components for testing are properly set up.

Consider organizing the imports for better readability. You can group them by their source:

// Play and ScalaTest imports
import org.scalatestplus.play._
import play.api.http.Status.{BAD_REQUEST, OK}
import play.api.mvc._
import play.api.test.Helpers._
import play.api.test._

// Circe imports
import io.circe._
import io.circe.generic.auto._
import io.circe.parser._

// Application-specific imports
import model.ListModelResponse
import org.scalatest.EitherValues

29-32: LGTM! Consider parameterized tests for edge cases.

This test case correctly verifies that the controller returns a BAD_REQUEST status when given a negative limit. It's consistent with the previous test for bad offset.

Consider using ScalaTest's property-based testing or table-driven tests to cover more edge cases efficiently. For example:

"send 400 on invalid input" in {
  val invalidInputs = Table(
    ("offset", "limit"),
    (Some(-1), Some(10)),
    (Some(10), Some(-2)),
    (Some(-1), Some(-1)),
    (Some(Int.MinValue), Some(Int.MaxValue))
  )

  forAll(invalidInputs) { (offset, limit) =>
    val result = controller.list(offset, limit).apply(FakeRequest())
    status(result) mustBe BAD_REQUEST
  }
}

This approach allows you to test multiple invalid inputs in a single test case, improving test coverage and readability.


34-42: LGTM! Good validation of response content.

This test case thoroughly checks the controller's behavior for a valid request with default parameters. It verifies both the response status and the content of the response body.

Consider adding a more descriptive error message for the JSON decoding step:

val listModelResponse: Either[Error, ListModelResponse] = decode[ListModelResponse](bodyText).valueOr(e => fail(s"Failed to decode JSON: ${e.getMessage}"))

This will provide more context if the JSON decoding fails, making it easier to debug potential issues.


44-54: LGTM! Good test for pagination. Consider additional assertions and edge cases.

This test case effectively verifies the pagination functionality of the controller. It checks the response status, number of items, and the correct ID range based on the provided offset and limit.

Consider the following improvements:

  1. Add assertions for the total and offset fields in the ListModelResponse:
val response = listModelResponse.value
response.total mustBe (startOffset + number) // Assuming total is the last ID
response.offset mustBe startOffset
  1. Test edge cases for pagination, such as:

    • Requesting more items than available
    • Requesting with an offset larger than the total number of items
    • Requesting with the maximum allowed limit
  2. Consider using ScalaTest's property-based testing to generate various combinations of offset and limit values:

import org.scalacheck.Gen
import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks

// ... in your test class
forAll(Gen.choose(0, 100), Gen.choose(1, 50)) { (offset, limit) =>
  val result = controller.list(Some(offset), Some(limit)).apply(FakeRequest())
  status(result) mustBe OK
  // ... rest of your assertions
}

These improvements will enhance the test coverage and robustness of your pagination tests.

hub/test/controllers/SearchControllerSpec.scala (3)

33-41: LGTM with suggestion: Comprehensive test for valid request.

This test case thoroughly checks the controller's behavior for a valid request, including:

  1. Verifying the 200 OK status
  2. Decoding the response body
  3. Checking the number of items matches the default limit
  4. Verifying the returned item IDs

Consider adding an assertion to check the total field in the ListModelResponse. This would ensure that the pagination metadata is also correct:

listModelResponse.value.total mustBe (expectedTotalCount)

Replace expectedTotalCount with the actual expected total count of models.


43-53: LGTM with suggestions: Good pagination test with room for improvement.

This test case effectively verifies the pagination functionality of the search endpoint. It checks:

  1. The correct HTTP status (200 OK)
  2. The number of items returned matches the requested limit
  3. The IDs of the returned items are in the expected range

Consider the following improvements:

  1. Add an assertion to check the total field in the ListModelResponse:

    listModelResponse.value.total mustBe (expectedTotalCount)

    Replace expectedTotalCount with the actual expected total count of models.

  2. Verify that the nextOffset field in the response is correct:

    listModelResponse.value.nextOffset mustBe Some(startOffset + number)
  3. Add a test case with a limit larger than the total number of items to ensure correct behavior when reaching the end of the list.

These additions would provide more comprehensive coverage of the pagination functionality.


1-55: Overall good test coverage with suggestions for expansion.

The SearchControllerSpec provides a solid foundation for testing the SearchController. It covers essential scenarios including error handling for invalid parameters, successful responses for valid requests, and basic pagination functionality.

Consider adding the following test cases to improve coverage:

  1. Test with empty search term to ensure correct handling.
  2. Test with very large offset and limit values to check for any upper bounds.
  3. Test with special characters or SQL injection attempts in the search term to ensure proper sanitization.
  4. Test the behavior when no results are found for a given search term.
  5. Verify that the search is case-insensitive (if that's a requirement).
  6. Test with the maximum allowed limit to ensure it's enforced correctly.

Adding these cases would provide more comprehensive coverage and help catch potential edge cases or security issues.

build.sbt (1)

30-30: Summary of build.sbt changes and suggestions for follow-up.

The changes to build.sbt are focused on adding Circe for JSON handling and updating the scalatestplus-play version. These modifications align well with the PR objectives of implementing stubbed backend APIs.

To ensure a smooth integration of these changes:

  1. Verify that the new Circe dependencies are properly utilized in the implementation of the stubbed APIs.
  2. Consider adding any necessary Circe-specific configurations to the project settings if required.
  3. After confirming test compatibility with the new scalatestplus-play version, update any relevant documentation about the project's testing setup.

Also applies to: 222-225

hub/app/model/Model.scala (1)

32-42: Consider clarifying the naming of Metric and its case objects.

To enhance clarity, you might consider renaming the Metric trait to better reflect its purpose, such as MetricRollup, since it represents different ways to aggregate metrics.

hub/app/controllers/TimeSeriesController.scala (2)

326-329: Potential Performance Issue with Large Time Ranges

In the generateMockTimeSeriesPoints method, creating a sequence over a large time range using (startTs until endTs by (1.hours.toMillis)) can lead to performance issues or memory exhaustion if the range between startTs and endTs is significant.

Consider adding a check to limit the number of points generated or adjusting the step size based on the range:

 def generateMockTimeSeriesPoints(startTs: Long, endTs: Long): Seq[TimeSeriesPoint] = {
   val random = new Random(1000)
+  val maxPoints = 1000
+  val step = ((endTs - startTs) / maxPoints).max(1.hours.toMillis)
-  (startTs until endTs by (1.hours.toMillis)).map(ts => TimeSeriesPoint(random.between(0, 0.5), ts))
+  (startTs until endTs by step).map(ts => TimeSeriesPoint(random.between(0, 0.5), ts))
 }

This limits the number of generated points to a manageable size.


258-271: Maintain Consistent Variable Naming for Clarity

In the method doFetchFeatureSkew, the variable name featuresTs is used, which is inconsistent with featureTs used elsewhere. This might cause confusion when reading the code.

Consider renaming featuresTs to featureTs for consistency:

-  val featuresTs = FeatureTimeSeries(name, generateMockTimeSeriesPercentilePoints(startTs, endTs))
-  featuresTs.asJson.noSpaces
+  val featureTs = FeatureTimeSeries(name, generateMockTimeSeriesPercentilePoints(startTs, endTs))
+  featureTs.asJson.noSpaces

This makes the variable naming consistent across the codebase.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 65a5e10 and 5446c70.

📒 Files selected for processing (11)
  • build.sbt (2 hunks)
  • hub/app/controllers/ApplicationController.scala (1 hunks)
  • hub/app/controllers/ModelController.scala (1 hunks)
  • hub/app/controllers/SearchController.scala (1 hunks)
  • hub/app/controllers/TimeSeriesController.scala (1 hunks)
  • hub/app/model/Model.scala (1 hunks)
  • hub/conf/application.test.conf (1 hunks)
  • hub/conf/routes (1 hunks)
  • hub/test/controllers/ModelControllerSpec.scala (1 hunks)
  • hub/test/controllers/SearchControllerSpec.scala (1 hunks)
  • hub/test/controllers/TimeSeriesControllerSpec.scala (1 hunks)
🔇 Additional comments (25)
hub/app/controllers/ApplicationController.scala (2)

7-10: LGTM: Class definition follows Play Framework conventions.

The ApplicationController class is well-structured:

  • It correctly extends BaseController.
  • Dependency injection is properly used for ControllerComponents.
  • The class-level comment provides a clear description of the controller's purpose.

1-15: Overall assessment: Well-implemented controller for basic health check.

The ApplicationController is correctly implemented and follows Play Framework conventions. It provides a simple ping endpoint which can be useful for basic health checks or testing connectivity.

Key points:

  1. Proper use of dependency injection and extension of BaseController.
  2. Clear and concise implementation of the ping() method.

Minor suggestions for improvement have been provided in previous comments, but these are not critical issues. The controller is functional and ready for use as is.

hub/conf/application.test.conf (1)

7-13: Verify the database configuration requirements.

The database configuration is currently commented out. If this is intentional for the test configuration, it's fine. However, if a database is required for testing, you'll need to uncomment and possibly modify these lines.

Could you confirm if:

  1. A database is needed for the tests?
  2. If needed, is H2 in-memory database the intended choice?
  3. Are there any specific database settings required for the test environment?
hub/app/controllers/ModelController.scala (3)

1-14: LGTM: Imports and class definition are well-structured.

The import statements and class definition follow best practices for Play Framework controllers. The use of dependency injection and the @Singleton annotation is appropriate.


28-29: LGTM: Default pagination values are well-defined.

The use of constants for default offset and limit values is a good practice. The values (0 for offset and 10 for limit) seem reasonable for initial pagination.


31-36: LGTM: Method signature and documentation are well-defined.

The list method signature is appropriate for a Play Framework controller action. The use of optional parameters for offset and limit allows for flexible API usage. The documentation clearly explains the method's purpose and parameters.

hub/test/controllers/ModelControllerSpec.scala (2)

24-27: LGTM! Good test for invalid offset.

This test case correctly verifies that the controller returns a BAD_REQUEST status when given a negative offset. It's a good practice to test edge cases like this.


1-56: Overall, well-structured and comprehensive test suite. Consider suggested improvements for enhanced coverage.

The ModelControllerSpec provides a solid foundation for testing the ModelController. It covers essential scenarios including error handling and pagination. The tests are well-organized and use appropriate testing utilities from Play Framework and ScalaTest.

To further improve the test suite, consider implementing the following suggestions:

  1. Organize imports for better readability.
  2. Use parameterized tests for edge cases, especially for input validation.
  3. Add more descriptive error messages for JSON decoding steps.
  4. Enhance pagination tests with additional assertions and edge cases.
  5. Explore property-based testing for more comprehensive input combination coverage.

These improvements will lead to a more robust and maintainable test suite, ensuring the reliability of the ModelController as the project evolves.

hub/test/controllers/SearchControllerSpec.scala (3)

1-20: LGTM: Imports and class setup look good.

The import statements are appropriate for the test suite, including necessary libraries for JSON handling, testing frameworks, and Play Framework utilities. The class declaration extends the required traits for testing, and the stub ControllerComponents is correctly set up for isolated testing of the SearchController.


23-26: LGTM: Good edge case testing for bad offset.

This test case correctly verifies that the controller returns a 400 BAD REQUEST status when provided with an invalid (negative) offset. This is an important edge case to cover in the test suite.


28-31: LGTM: Good edge case testing for bad limit.

This test case properly verifies that the controller returns a 400 BAD REQUEST status when provided with an invalid (negative) limit. This is another important edge case to cover in the test suite.

hub/app/controllers/SearchController.scala (2)

1-9: LGTM: Package declaration and imports are appropriate.

The package declaration and imports are well-structured and include all necessary components for the controller's functionality. The use of Circe for JSON handling is a good choice for Scala projects.


10-13: LGTM: Controller class declaration is well-structured.

The SearchController class is correctly defined with proper dependency injection and extends BaseController as expected in a Play Framework application.

build.sbt (3)

30-30: LGTM: Addition of circeVersion.

The addition of circeVersion as a lazy val is a good practice for managing dependency versions in sbt. The version "0.14.9" appears to be the latest stable version of Circe at the time of this review.


223-225: LGTM: Addition of Circe dependencies.

The addition of Circe dependencies (core, generic, and parser) is appropriate for implementing the stubbed backend APIs mentioned in the PR objectives. Circe is a well-regarded library for JSON handling in Scala. Using the previously defined circeVersion for all three dependencies ensures version consistency, which is a good practice.


222-222: Verify compatibility with updated scalatestplus-play version.

The update from version "5.1.0" to "7.0.0" for scalatestplus-play is a significant version jump. While keeping dependencies up-to-date is generally good practice, please ensure that all existing tests are compatible with this new version.

To verify the compatibility, please run the test suite and confirm that all tests pass with the new version. You can use the following command:

hub/conf/routes (2)

2-4: Endpoints are correctly defined

The routes for /api/v1/ping, /api/v1/models, and /api/v1/search are properly specified and align with the expected functionality.


7-10: Endpoints are correctly defined

The routes for /api/v1/join, /api/v1/join/slice, /api/v1/feature, and /api/v1/feature/slice are properly specified and consistent with the application's API design.

hub/app/model/Model.scala (6)

4-11: The Model case class is well-structured and comprehensive.

The Model class accurately captures all necessary attributes for machine learning models in Zipline.


14-21: Definition of MetricType and its variants is appropriate.

The sealed trait MetricType with Drift and Skew case objects is well-defined and allows for clear type safety.


23-30: DriftAlgorithm trait and its implementations are correctly specified.

The sealed trait DriftAlgorithm with PSI and KL case objects appropriately represents the supported drift algorithms.


44-54: Granularity trait and its variants are well-defined.

The sealed trait Granularity with Raw, Percentile, and Aggregates case objects effectively represents the different data granularity levels.


55-58: Time series case classes are properly constructed.

The case classes TimeSeriesPoint, FeatureTimeSeries, RawComparedFeatureTimeSeries, and GroupByTimeSeries are well-defined for handling time series data structures.


61-65: Response case classes are appropriately designed for API endpoints.

The response classes SearchModelResponse, ListModelResponse, ModelTimeSeriesResponse, and JoinTimeSeriesResponse are correctly structured to facilitate API responses.

hub/app/controllers/TimeSeriesController.scala (1)

293-300: Verify the Mapping of "ooc" to Skew in parseMetricChoice

In the parseMetricChoice method, the string "ooc" is mapped to Some(Skew):

case Some("ooc")   => Some(Skew)

Ensure that mapping "ooc" (possibly "out of control") to Skew is intentional and accurately reflects the domain logic.

Run the following script to check for other usages of "ooc" and how it's handled:

This will help confirm whether "ooc" should map to Skew or if it requires a separate handling or enumeration.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5446c70 and 6e88890.

📒 Files selected for processing (6)
  • hub/app/controllers/ModelController.scala (1 hunks)
  • hub/app/controllers/SearchController.scala (1 hunks)
  • hub/app/controllers/TimeSeriesController.scala (1 hunks)
  • hub/test/controllers/ModelControllerSpec.scala (1 hunks)
  • hub/test/controllers/SearchControllerSpec.scala (1 hunks)
  • hub/test/controllers/TimeSeriesControllerSpec.scala (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • hub/app/controllers/ModelController.scala
  • hub/app/controllers/SearchController.scala
  • hub/test/controllers/ModelControllerSpec.scala
  • hub/test/controllers/SearchControllerSpec.scala
  • hub/test/controllers/TimeSeriesControllerSpec.scala
🔇 Additional comments (10)
hub/app/controllers/TimeSeriesController.scala (10)

1-17: Class structure and dependencies look good

The TimeSeriesController is well-structured with appropriate imports and dependency injection. The use of @Singleton annotation ensures a single instance of the controller.


45-52: LGTM: Well-structured method with appropriate optional parameters

The fetchJoin method is well-implemented with correct parameter types and delegation to the private doFetchJoin method.


58-66: LGTM: Consistent implementation with fetchJoin

The fetchJoinSlice method is correctly implemented, maintaining consistency with the fetchJoin method while adding the sliceId parameter.


73-81: LGTM: Well-implemented method with appropriate parameters

The fetchFeature method is correctly implemented with a good balance of required and optional parameters, delegating to the private doFetchFeature method.


87-96: LGTM: Consistent implementation with fetchFeature

The fetchFeatureSlice method is correctly implemented, maintaining consistency with the fetchFeature method while adding the sliceId parameter.


123-141: LGTM: Well-structured method with proper error handling

The doFetchJoin method is well-implemented with appropriate parsing of parameters and delegation to specific methods based on the metric type.


177-194: LGTM: Straightforward implementation of mock data generation

The doFetchJoinSkew method is correctly implemented, generating appropriate mock data for join skew time series.


196-218: LGTM: Well-structured method with proper error handling and delegation

The doFetchFeature method is well-implemented with appropriate parsing of parameters and delegation to specific methods based on the metric type and granularity.


251-271: LGTM: Well-structured implementation with proper granularity handling

The doFetchFeatureSkew method is correctly implemented, handling different granularity types and generating appropriate mock data for feature skew time series.


321-349: LGTM: Well-implemented mock data generation methods

The mock data generation methods are correctly implemented, providing appropriate test data for various scenarios. These methods will be useful for testing and development until the actual data retrieval is implemented.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
hub/app/controllers/Paginate.scala (2)

5-9: LGTM: Well-defined trait with appropriate constants.

The Paginate trait is well-named and includes useful constants for pagination. The maxLimit of 100 is a good practice to prevent excessive data retrieval.

Consider adding Scaladoc comments to explain the purpose of these constants, especially maxLimit, to improve code documentation:

 trait Paginate {
+  /** Default offset for pagination, starts from the beginning of the list. */
   val defaultOffset = 0
+  /** Default limit for the number of items per page. */
   val defaultLimit = 10
+  /** Maximum allowed limit to prevent excessive data retrieval. */
   val maxLimit = 100
 }

10-12: Approve with suggestions: Add input validation and handle edge cases.

The paginateResults method correctly implements basic pagination using slice. However, it lacks input validation and doesn't handle edge cases.

Consider adding input validation and handling edge cases:

def paginateResults(results: Seq[Model], offset: Int, limit: Int): Seq[Model] = {
  val validatedOffset = Math.max(0, offset)
  val validatedLimit = Math.min(maxLimit, Math.max(1, limit))
  results.slice(validatedOffset, validatedOffset + validatedLimit)
}

This implementation:

  1. Ensures the offset is non-negative.
  2. Ensures the limit is between 1 and maxLimit.
  3. Prevents potential issues with negative or excessively large inputs.

Additionally, consider adding a comment explaining the method's behavior:

/**
 * Paginates the given sequence of Model objects.
 *
 * @param results The sequence of Model objects to paginate
 * @param offset The starting index for pagination (must be non-negative)
 * @param limit The maximum number of items to return (capped at maxLimit)
 * @return A subsequence of Model objects based on the given offset and limit
 */
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6e88890 and bdb5136.

📒 Files selected for processing (6)
  • hub/app/controllers/ModelController.scala (1 hunks)
  • hub/app/controllers/Paginate.scala (1 hunks)
  • hub/app/controllers/SearchController.scala (1 hunks)
  • hub/app/controllers/TimeSeriesController.scala (1 hunks)
  • hub/app/model/Model.scala (1 hunks)
  • hub/test/controllers/TimeSeriesControllerSpec.scala (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • hub/app/controllers/ModelController.scala
  • hub/app/controllers/SearchController.scala
  • hub/app/controllers/TimeSeriesController.scala
  • hub/app/model/Model.scala
  • hub/test/controllers/TimeSeriesControllerSpec.scala
🔇 Additional comments (1)
hub/app/controllers/Paginate.scala (1)

1-4: LGTM: Package declaration and imports are appropriate.

The package declaration and import are correctly set up for a controller-related trait that works with model objects.

Copy link
Contributor

@ken-zlai ken-zlai left a comment

Choose a reason for hiding this comment

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

This should have what I need to start wiring up the home page. Will be great to have official swagger docs as a follow up :)

@piyush-zlai piyush-zlai merged commit c6717b1 into main Sep 30, 2024
7 checks passed
@piyush-zlai piyush-zlai deleted the piyush--apis branch September 30, 2024 18:11
This was referenced Oct 1, 2024
This was referenced Nov 5, 2024
@coderabbitai coderabbitai bot mentioned this pull request Feb 7, 2025
4 tasks
kumar-zlai pushed a commit that referenced this pull request Apr 25, 2025
Implement stubbed backend APIs for model list, search and time series
kumar-zlai pushed a commit that referenced this pull request Apr 29, 2025
Implement stubbed backend APIs for model list, search and time series
chewy-zlai pushed a commit that referenced this pull request May 16, 2025
Implement stubbed baour clientsend APIs for model list, search and time series
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.

2 participants