-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add HTTP API version 1 #91
Conversation
Please ignore the *Dtos for now. Since the fields for the response are not fixed, I just used POJO withe getters and setters without much thinking about it. |
babd684
to
d26fd79
Compare
* <p>Adds or edit a file. | ||
*/ | ||
@Put("regex:/projects/(?<projectName>[^/]+)/repos/(?<repoName>[^/]+)/contents(?<path>/.*$)") | ||
public CompletionStage<EntryDto> addOrEditFile( |
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.
Because there's an input for revision, this is not idempotent. If I change this using POST, it does not make sense to edit file. Do you have an idea for this?
Or change the logic which returns just 200 OK when there's no change between the files and the head revision?
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.
I think it's OK to use POST here. Why doesn't it make sense to use POST for editing a file? We could 'post' a file modification request and the server could process it immediately?
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.
I have read the spec again, and it's fine using POST here. :-)
((ObjectNode) jsonNode).remove("url"); | ||
final HttpHeaders headers = HttpHeaders.of(HttpStatus.CREATED) | ||
.add(HttpHeaderNames.LOCATION, url) | ||
.addObject(HttpHeaderNames.CONTENT_TYPE, |
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.
Do we need convenience methods for this on upstream? like HttpResponse.of(HttpHeaders .....)?
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.
HttpRequest
seems to have one. It would be great to have something similar in HttpResponse
as well. Please send a PR.
* | ||
* <p>Deletes a file. | ||
*/ | ||
@Delete("regex:/projects/(?<projectName>[^/]+)/repos/(?<repoName>[^/]+)/contents(?<path>/.*$)") |
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.
Do we need convenience methods for this on upstream? like HttpClient.delete(uri, string body)? It is not a usual case though.
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.
Like adding Repository.deleteFiles(Revision revision, String pathPattern)
? Sounds good to me.
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 57.25% 58.94% +1.68%
==========================================
Files 229 249 +20
Lines 7971 8498 +527
Branches 933 994 +61
==========================================
+ Hits 4564 5009 +445
- Misses 2830 2877 +47
- Partials 577 612 +35
Continue to review full report at Codecov.
|
* Returns the {@link Markup} from the specified {@code value}. If none of markup is matched, | ||
* this will return {@link #UNKNOWN}. | ||
*/ | ||
public static Markup getMarkup(@Nullable String value) { |
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.
parse
or of
?
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. Thank you~!
} | ||
|
||
public static ArrayNode createArrayNode() { | ||
return compactMapper.createArrayNode(); |
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.
You could just use JsonNodeFactory.instance.*Node()
?
@@ -165,6 +167,15 @@ public static String writeValueAsPrettyString(Object value) throws JsonProcessin | |||
return compactMapper.convertValue(fromValue, toValueTypeRef); | |||
} | |||
|
|||
public static ObjectNode createObjectNode() { | |||
compactMapper.createArrayNode(); | |||
return compactMapper.createObjectNode(); |
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.
You could just use JsonNodeFactory.instance.*Node()
?
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. Thank you~!
|
||
private void checkRepositoryExists(String projectName, String repoName) { | ||
if (!projectManager().exists(projectName)) { | ||
throw new HttpResponseException(HttpStatus.BAD_REQUEST); |
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.
NOT_FOUND
?
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. Thank you~!
} | ||
|
||
if (!projectManager().get(projectName).repos().exists(repoName)) { | ||
throw new HttpResponseException(HttpStatus.BAD_REQUEST); |
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.
NOT_FOUND
?
|
||
private static Change<?> getChangeOfPatch(String path, JsonNode jsonNode) { | ||
if (path.charAt(path.length() - 1) == '/') { | ||
throw new HttpResponseException(HttpStatus.BAD_REQUEST); |
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.
Perhaps we could rely on Util.validateFilePath()
where necessary?
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. Thank you~!
((ObjectNode) jsonNode).remove("url"); | ||
final HttpHeaders headers = HttpHeaders.of(HttpStatus.CREATED) | ||
.add(HttpHeaderNames.LOCATION, url) | ||
.addObject(HttpHeaderNames.CONTENT_TYPE, |
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.
HttpRequest
seems to have one. It would be great to have something similar in HttpResponse
as well. Please send a PR.
@Param("repoName") String repoName) { | ||
checkProjectExists(projectName); | ||
final long removeTimeMillis = System.currentTimeMillis(); | ||
return execute(Command.removeRepository(removeTimeMillis, AuthenticationUtil.currentAuthor(), |
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.
You can just use the factory method without timestamp parameter to let the upstream decide.
final String repoName = jsonNode.asText(); | ||
checkArgument(!isNullOrEmpty(repoName), "repository name should be non-null"); | ||
|
||
return execute(Command.createRepository(System.currentTimeMillis(), AuthenticationUtil.currentAuthor(), |
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.
You can just use the factory method without timestamp parameter to let the upstream decide.
throw new HttpResponseException(HttpStatus.BAD_REQUEST); | ||
} | ||
|
||
return execute(Command.unremoveRepository(System.currentTimeMillis(), |
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.
You can just use the factory method without timestamp parameter to let the upstream decide.
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.
I was planning to use the value to return the client at first. I will remove them for now, and bring it back if it needs. :-)
6776c93
to
4a47f24
Compare
} | ||
|
||
/** | ||
* POST /projects/{projectName}/repos/{repoName}/contents{path}?revision={revision} |
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.
Looks different from the implementation. Which one is correct?
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.
I didn't change the path when it's changed from PUT to POST, thank you~!
} | ||
|
||
/** | ||
* GET /projects/{projectName}/repos/{repoName}/contents{path}?revision={revision} |
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.
revision={revision}
-> revision={revision}&
?
fbb8090
to
6130fa9
Compare
Motivation: The libthrift is not stable enough yet, we need two diffrent jars to compile. If we use HTTP API, we can avoid that situation and have fewer depedencies for the client library. Modifications: - Revise admin REST APIs - Add more APIs to cover all of the features in Thrift service Result: - We are ready to transfer from Thrift to HTTP API Todos: - Support adding and editing for multiple files - Support preview diffs - Create URLs with URI template parser - Remove admin package or keep it minumun - Remove thrift service ultimately
2542bc4
to
0f8bc09
Compare
* <p>Adds or edits a file. | ||
*/ | ||
@Post("/projects/{projectName}/repos/{repoName}/contents") | ||
public CompletionStage<EntryDto> addOrEditFile(@Param("projectName") String projectName, |
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.
nit: EntryDto
-> EntryDto<?>
?
*/ | ||
public CompletableFuture<Revision> watchRepository(Repository repo, Revision lastKnownRevision, | ||
String pathPattern, long timeoutMillis) { | ||
if (isServerStopping()) { |
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.
Just curious. Is this kind of prevention required for every API calls? I mean that do we have something like a decorator which sends a failure response when the server starts stopping?
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.
Just curious. Is this kind of prevention required for every API calls? I mean that do we have something like a decorator which sends a failure response when the server starts stopping?
Sounds like a good idea which is worth its own issue. Could you create one?
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.
Created! #107
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.
Thanks, @hyangtack !
return projectManager().get(projectName).repos().get(repoName); | ||
} | ||
|
||
private void checkRepositoryExists(String projectName, String repoName) { |
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.
How about returning Repository
here so that we can call this once to validate the name and get Repository
instead of calling getRepository
again?
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.
Or, we may move this kind of codes to a request converter later.
AggregatedHttpMessage message) { | ||
checkRepositoryExists(projectName, repoName); | ||
|
||
final Entry<CommitMessageDto, Change<?>> commitMessageAndChange = commitMessageAndChange(message); |
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.
Try to use a request converter later! :-)
@Param("revision") @Default("-1") String revision, | ||
@Param("queryType") Optional<String> queryType, | ||
@Param("expression") Optional<String> expression, | ||
@Header("if-none-match") Optional<String> ifNoneMatch, |
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.
Try to use a request converter here, too. :-) I think it'd be better to make an object like WatchRequest
so that we can move the codes like parsing timeout to the request converter.
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.
I realized that we need to make RequestObject
as optional for this. I'm not sure if it's good idea though. WDYT @hyangtack ?
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.
I think we can return Optional<?>
from a request converter. Not possible?
final JsonNode jsonNode = p.readValueAsTree(); | ||
final JsonNode summary = jsonNode.get("summary"); | ||
if (summary.textValue() == null) { | ||
ctxt.reportInputMismatch(CommitMessageDto.class, "commit message should hava summary"); |
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.
nit: hava -> have
this.type = requireNonNull(type, "type"); | ||
this.content = null; | ||
this.revision = requireNonNull(revision, "revision"); | ||
url = PROJECTS_PREFIX + '/' + projectName + REPOS + '/' + repoName + path; |
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.
I think it'd be better to remove the logic which generates a URL from DTO class. :-)
@@ -31,7 +32,8 @@ | |||
|
|||
@JsonCreator | |||
IdentityQuery(@JsonProperty("path") String path) { | |||
this.path = requireNonNull(path, "path"); | |||
requireNonNull(path, "path"); |
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.
requireNonNull
is redundant because validateFilePath()
does null check as well.
@@ -41,7 +42,7 @@ | |||
requireNonNull(path, "path"); |
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.
requireNonNull
is redundant because validateJsonFilePath()
does null check as well.
@@ -52,7 +53,7 @@ | |||
requireNonNull(path, "path"); |
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.
requireNonNull
is redundant because validateJsonFilePath()
does null check as well.
@@ -38,6 +38,8 @@ | |||
"^(?:[-_0-9a-zA-Z](?:[-_\\.0-9a-zA-Z]*[-_0-9a-zA-Z])?)+$"); | |||
private static final Pattern FILE_PATH_PATTERN = Pattern.compile( | |||
"^(?:/[-_0-9a-zA-Z](?:[-_\\.0-9a-zA-Z]*[-_0-9a-zA-Z])?)+$"); | |||
private static final Pattern JSON_FILE_PATH_PATTERN = Pattern.compile( | |||
"^(?:/[-_0-9a-zA-Z](?:[-_\\.0-9a-zA-Z]*[-_0-9a-zA-Z])?)+\\.(?i)json$"); |
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.
Do we treat .JSON
as JSON? IIRC we only accept .json
, but my memory may not be correct.
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.
I did not know. That was just my decision. I will remove the (?i)
:-)
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.
Actually, it seems case-insensitive. In EntryType
:
/**
* Guesses the {@link EntryType} from the specified {@code path}.
*/
public static EntryType guessFromPath(String path) {
requireNonNull(path, "path");
if (path.isEmpty()) {
throw new IllegalArgumentException("empty path");
}
if (path.charAt(path.length() - 1) == '/') {
return DIRECTORY;
}
if (Ascii.toLowerCase(path).endsWith(".json")) {
return JSON;
}
return TEXT;
}
Sorry!
return isValidDirPath(path, false); | ||
} | ||
|
||
public static boolean isValidDirPath(String path, boolean mustEndsWithSlash) { |
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.
mustEnds -> mustEnd
if (thrown != null) { | ||
if (Throwables.getRootCause(thrown) instanceof StorageExistsException) { | ||
throw newHttpResponseException(HttpStatus.BAD_REQUEST, | ||
"project " + name + " already exists"); |
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.
Perhaps CONFLICT
?
if (!jsonNode.equals(unremovePatch)) { | ||
throw newHttpResponseException(HttpStatus.BAD_REQUEST, | ||
"not supported JSON patch: " + message.content() + | ||
" (expected: " + unremovePatch.toString() + ')'); |
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.
Maybe CONFLICT
?
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.
Is this CONFLICT
? This patch
method only supports unremovePatch
for now, so if the request is sent except the patch
, I thought we need to give back BAD_REQUEST
.
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.
I think there are two cases involved here.
- The JSON document is a JSON patch but is not equal to
unremovePatch
, which would beCONFLICT
. - The JSON document is not a JSON patch, which would be
BAD_REQUEST
.
Could we send BAD_REQUEST
when it's not a JSON patch or CONFLICT
otherwise? You could try to deserialize the content into JsonPatch
to test if it's a JSON patch.
if (thrown != null) { | ||
if (Throwables.getRootCause(thrown) instanceof StorageExistsException) { | ||
throw newHttpResponseException(HttpStatus.BAD_REQUEST, | ||
"repository " + repoName + " already exists"); |
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.
CONFLICT
?
if (!unremovePatch.equals(jsonNode)) { | ||
throw newHttpResponseException(HttpStatus.BAD_REQUEST, | ||
"not supported JSON patch: " + message.content() + | ||
" (expected: " + unremovePatch.toString() + ')'); |
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.
CONFLICT
?
*/ | ||
public CompletableFuture<Revision> watchRepository(Repository repo, Revision lastKnownRevision, | ||
String pathPattern, long timeoutMillis) { | ||
if (isServerStopping()) { |
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.
Just curious. Is this kind of prevention required for every API calls? I mean that do we have something like a decorator which sends a failure response when the server starts stopping?
Sounds like a good idea which is worth its own issue. Could you create one?
d235066
to
75b565b
Compare
|
||
import com.linecorp.centraldogma.common.ChangeType; | ||
|
||
@JsonInclude(Include.NON_DEFAULT) |
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.
Is this intentional? @JsonInclude(Include.NON_NULL)
?
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.
Yeah~ the content
can be null. I just wanted the client gets only path
and type
when the content
is null. :-)
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.
Left some nits
return IDENTITY; | ||
} | ||
|
||
final String queryType = Ascii.toUpperCase(value); |
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.
Seems like uppercasing here is unnecessary because you used equalsIgnoreCase
?
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.
Oops~!!
return JSON_PATH; | ||
} | ||
|
||
return IDENTITY; |
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.
Shouldn't we raise an exception or return null?
|
||
@Override | ||
public String meterTag() { | ||
return "hostnameService"; |
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.
Just hostname
?
/** | ||
* A service class for watching repository or a file. | ||
*/ | ||
public class WatchService { |
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.
nit: Probably OK to make *Service
classes final
?
Thank you, @minwoox ! |
Motivation:
The libthrift is not stable enough yet, we need two diffrent jars to compile.
If we use HTTP API, we can avoid that situation and have fewer depedencies for the client library.
Modifications:
Result:
Todos:
MetaDataService