Conversation
a534aa6 to
e14dc2a
Compare
| mock-data: | ||
| enabled: true | ||
| mapping: "/mock-data/**" | ||
| paths: "classpath:schemas" |
There was a problem hiding this comment.
Can we please rename the actual folder src/test/resources/schemas to mock-data please?
| request, | ||
| HttpResponse.BodyHandlers.ofInputStream() | ||
| ); | ||
| try (CloseableHttpResponse response = httpClient.execute(httpGet)) { |
There was a problem hiding this comment.
Make variable as final please
| ); | ||
| try (CloseableHttpResponse response = httpClient.execute(httpGet)) { | ||
|
|
||
| int statusCode = response.getStatusLine().getStatusCode(); |
There was a problem hiding this comment.
Mark variable as final please
|
|
||
| try (InputStream inputStream = response.body()) { | ||
| Files.copy(inputStream, tempFile, StandardCopyOption.REPLACE_EXISTING); | ||
| try (InputStream inputStream = response.getEntity().getContent()) { |
There was a problem hiding this comment.
Mark variable as final please
| .followRedirects(HttpClient.Redirect.NORMAL) | ||
| this.poolingHttpClientConnectionManager = new PoolingHttpClientConnectionManager(); | ||
|
|
||
| this.httpRequestConfig = RequestConfig.custom() |
There was a problem hiding this comment.
You also need:
poolingHttpClientConnectionManager.setMaxTotal(20);
poolingHttpClientConnectionManager.setDefaultMaxPerRoute(15);
poolingHttpClientConnectionManager.setValidateAfterInactivity(15_000);| .setConnectionRequestTimeout(3_000) | ||
| .build(); | ||
|
|
||
| this.httpClient = HttpClients |
There was a problem hiding this comment.
You need to build a new client for each request - this should not be in the constructor, it needs to be done in downloadToTemp - see the https://github.com/evolvedbinary/expath-package-repository-plugin/blob/main/src/main/java/com/evolvedbinary/maven/plugins/expath/pkg/repository/ResolveMojo.java as an example
| <artifactId>micronaut-reactor</artifactId> | ||
| <version>3.9.1</version> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
If this is just an example - please remove this commit or merge it into the final relevant view commit
There was a problem hiding this comment.
if you delete this commit from the history, when doing a git interactive rebase, your following commit (6540ac7) that removes this should be automatically skipped and excluded
| @View("validate") | ||
| @Get("/validate") | ||
| public Map<String, Object> validate() { | ||
| Map<String, Object> model = new HashMap<>(); |
There was a problem hiding this comment.
Please mark all variables, parameters, and class fields as final where possib;e
| @Post(value = "/validate", consumes = MediaType.APPLICATION_FORM_URLENCODED) | ||
| public Map<String, Object> validateSubmit(@Body Map<String, String> formData) { | ||
| String schemaId = formData.get("schemaId"); | ||
| String csvContent = formData.get("csvContent"); |
There was a problem hiding this comment.
If required parameters are not present, this should generate a BadRequest, or if going back to a HTML page then it should alert the user to the error
| // } | ||
|
|
||
| try { | ||
| Path tempFile = fileDownloadService.saveContentToTemp(csvContent); |
There was a problem hiding this comment.
Please make sure to delete/close any temporary resources that you own once you are finished with them! e.g. tempFile here.
| version: ${project.version} | ||
| schema: | ||
| directory: schemas No newline at end of file | ||
| directory: C:\Users\x\Documents\bbl-validator\src\main\resources\schemas No newline at end of file |
There was a problem hiding this comment.
Should not be hard-coded to your local environment
|
|
||
| try { | ||
| Path tempFile = fileDownloadService.saveContentToTemp(csvContent); | ||
| Path tempFile = isUrl ? fileDownloadService.downloadToTemp(csvUrl) : fileDownloadService.saveContentToTemp(csvContent); |
There was a problem hiding this comment.
Please make sure to cleanup your temporary resources
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <link rel="stylesheet" href="https://cdn.simplecss.org/simple.min.css"> | ||
| <title>BBL Validator</title> | ||
| <style> |
There was a problem hiding this comment.
It feels to me that this would be better in a seperate .css file that is <link from here please
| static-resources: | ||
| default: | ||
| enabled: true | ||
| mapping: "/**" |
There was a problem hiding this comment.
Change mapping to from /** -> /static/**
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <link rel="stylesheet" href="https://cdn.simplecss.org/simple.min.css"> | ||
| <link rel="stylesheet" href="/css/simple.min.css"> |
| @@ -0,0 +1,49 @@ | |||
| # Builder stage | |||
There was a problem hiding this comment.
You should add docker-maven-plugin to your pom.xml (see: https://github.com/evolvedbinary/elemental/blob/main/exist-docker/pom.xml#L276)
You can then template this Dockerfile, using Maven by moving it into src/main/resources - you can then use Maven Variables in your Dockerfile for the version number and anything else you need.
When you then run maven package it will also build your Docker image for you.
| FROM eclipse-temurin:21-jre-alpine | ||
|
|
||
| # metadata | ||
| LABEL maintainer="Evolved Binary <https://www.evolvedbinary.com>" |
There was a problem hiding this comment.
Can you update these to the OpenContainer format for labels please - see: https://github.com/evolvedbinary/elemental/blob/main/exist-docker/src/main/resources-filtered/Dockerfile#L74C1-L81C61
| @@ -0,0 +1,322 @@ | |||
| openapi: 3.1.0 | |||
There was a problem hiding this comment.
We should serve this as a static file, from our view page - we should have a link to view our API documentation - either "YAML"(link to the static file) or "Swagger" (link to the remote Swagger OpenAPI spec we built)
| @@ -81,8 +81,8 @@ paths: | |||
| schema: | |||
| type: string | |||
| description: The CSV Schema | |||
| '404': | |||
| $ref: '#/components/responses/NotFound' | |||
| '400': | |||
There was a problem hiding this comment.
If there is no such resource - that is a 404 not a 400, please fix.
| executionTime: | ||
| type: number | ||
| format: double | ||
| executionTimeMs: |
There was a problem hiding this comment.
Fix the code not the label in the OpenAPI spec please
| type: boolean | ||
| description: True if the Boradband Label file validated against the Schema, false otherwise | ||
| failures: | ||
| description: True if the CSV file is valid UTF-8 encoded |
There was a problem hiding this comment.
CSV file -> Broadband Label file
| description: True if the Boradband Label file validated against the Schema, false otherwise | ||
| failures: | ||
| description: True if the CSV file is valid UTF-8 encoded | ||
| errors: |
There was a problem hiding this comment.
Please change this back to how it was.
|
|
||
| ValidationFailure: | ||
| ValidationError: |
| type: object | ||
| required: | ||
| - line | ||
| - column | ||
| - lineNumber |
There was a problem hiding this comment.
Please fix the code here and not the schema
ef83bb0 to
9ca3b38
Compare
… a new http client for each request
…atches the expected API response
This PR primarily Adds the validation web view, but since then this branch diverted more toward the new
developbranch.Here's everything this PR is supposed to deliver:
PS: the review process already started up to this commit 1de92e4