-
Notifications
You must be signed in to change notification settings - Fork 6
Capture side endpoint refactored separately #158
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
Conversation
| content = @Content), | ||
| @ApiResponse(responseCode = "500", description = "Internal server error, contact admin", content = @Content) | ||
| }) | ||
| public ResponseEntity<?> getRelp(@PathVariable("id") int id, @RequestParam(required = false) Integer version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why suffix Relp? is get not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was accidentally left behind. Corrected.
| content = {@Content(mediaType = "application/json", | ||
| schema = @Schema(implementation = CaptureRelp.class))}), | ||
| }) | ||
| public List<CaptureRelp> getAllRelp(@RequestParam(required = false) Integer version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why suffix Relp? is get not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as get endpoint
| */ | ||
| USE cfe_18; | ||
| DELIMITER // | ||
| CREATE OR REPLACE PROCEDURE select_all_cfe_captures(tx_id INT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create an issue about renaming cfe -> file on the sql and mapper resultsets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
| @ExtendWith(MigrateDatabaseExtension.class) | ||
| public class CaptureControllerTest extends TestSpringBootInformation { | ||
| class CaptureFileControllerTest extends TestSpringBootInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public final please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
src/test/java/com/teragrep/cfe18/controllerTests/CaptureFileControllerTest.java
Show resolved
Hide resolved
| @Order(5) | ||
| public void testgetAllCaptures() throws Exception { | ||
| @Order(3) | ||
| public void testgetAllCfeCaptures() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File, not Cfe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| // Entity response string | ||
| String response2 = EntityUtils.toString(entity2); | ||
|
|
||
| // Parsin respponse as JSONObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
src/test/java/com/teragrep/cfe18/controllerTests/CaptureRelpControllerTest.java
Show resolved
Hide resolved
| request1.setHeader("Authorization", "Bearer " + token); | ||
|
|
||
| // Get the response from endpoint | ||
| HttpClientBuilder.create().build().execute(request1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response assertion missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asserted
| stmnt.execute(); | ||
| }); | ||
| Assertions.assertEquals("42000", state.getSQLState()); | ||
| Assertions.assertEquals("45000", state.getSQLState()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why expected sqls state code changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously had 42000 in the custom error handling. That was wrong since 42000 is reserved for MariaDB own specific errors.
For customized errors, the recommended SQLSTATE is '45000'.
Source : https://mariadb.com/kb/en/signal/
Pull request #155 has more in-detail comments about changes.