feature:add validate #114

jameszhangyukun · 2023-02-23T09:50:44Z

Ⅰ. Describe what this PR did

add validate for backend interface

Ⅱ. Does this pull request fix one issue?

#95

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

CH3CHO · 2023-02-23T10:03:39Z

backend/src/main/java/com/alibaba/higress/console/controller/DomainsController.java

@@ -56,6 +56,9 @@ public ResponseEntity<PaginatedResponse<Domain>> list(CommonPageQuery query) {

    @PostMapping
    public ResponseEntity<Response<Domain>> add(@RequestBody Domain domain) {
+        if (!domain.valid()) {
+            throw new ValidationException("Domain is invalid.");


Is it possible to let caller know which part is invalid and why?

If we want to know the reason. we need the valid() method return with some message

CH3CHO · 2023-02-23T10:04:52Z

backend/src/main/java/com/alibaba/higress/console/controller/dto/Domain.java

+            return false;
+        }
+        if (StringUtils.isNotEmpty(enableHttps)) {
+            return EnableHttps.OFF.equals(enableHttps) || EnableHttps.ON.equals(enableHttps)


If new fields are added to this class, is it still OK to return here?
And shall we check certificateId if user enables HTTPS?

I think if we add field in EnableHttps this may be not work. So we use eunum to replace EnableHttps

@Getter @AllArgsConstructor public enum EnableHttps { OFF("off"), ON("on"), FORCE("force"); private final String value; public static EnableHttps getEnum(String value) { for (EnableHttps enableHttps : values()) { if (Objects.equals(enableHttps.value, value)) { return enableHttps; } } return null; } }

then use getEnum to check enableHttps ?
if we neet to check certificateId if enableHttps. we can use TlsCertificateService in DomainsController then use this to check?

TlsCertificate certificate = tlsCertificateService.query(domain.getCertIdentifier()); if (Objects.isNull(certificate)) { throw new ValidationException("domain certificate is not exits."); }

You can have a try with the enableHttps change.

Maybe performing validation in the service is a better idea.

backend/src/main/java/com/alibaba/higress/console/controller/dto/Route.java

backend/src/main/java/com/alibaba/higress/console/controller/util/ValidateUtil.java

backend/src/main/java/com/alibaba/higress/console/util/TypeUtil.java

backend/src/main/java/com/alibaba/higress/console/controller/util/ValidateUtil.java

backend/src/main/java/com/alibaba/higress/console/controller/dto/TlsCertificate.java

backend/src/main/java/com/alibaba/higress/console/controller/dto/Route.java

CH3CHO · 2023-02-23T13:53:52Z

backend/src/main/java/com/alibaba/higress/console/controller/dto/Domain.java

+            return false;
+        }
+        if (StringUtils.isNotEmpty(enableHttps)) {
+            return EnableHttps.OFF.equals(enableHttps) || EnableHttps.ON.equals(enableHttps)


You can have a try with the enableHttps change.

Maybe performing validation in the service is a better idea.

jameszhangyukun · 2023-02-23T14:25:42Z

sorry, I fix in my side. but i do not push it to origin.

CH3CHO · 2023-02-25T07:42:27Z

backend/src/main/java/com/alibaba/higress/console/controller/dto/TlsCertificate.java

+        if (Objects.isNull(validityStart) || Objects.isNull(validityEnd)) {
+            return false;
+        }
+        return validityEnd.isAfter(validityStart);


domains, validityStart and validityEnd are extracted from cert data. Client doesn't submit them.

CH3CHO · 2023-02-25T07:42:47Z

backend/src/main/java/com/alibaba/higress/console/controller/TlsCertificatesController.java

        if (StringUtils.isNotEmpty(certificate.getName())) {
            certificate.setName(certificateName);
        } else if (!StringUtils.equals(certificateName, certificate.getName())) {
            throw new ValidationException("TlsCertificate name in the URL doesn't match the one in the body.");
        }
+        if (!certificate.valid()) {
+            throw new ValidationException("certificate is not valid.");


Is it possible to let user know why it is invalid?

backend/src/main/java/com/alibaba/higress/console/controller/DomainsController.java

CH3CHO · 2023-02-27T07:27:50Z

backend/src/main/java/com/alibaba/higress/console/controller/DomainsController.java

+        if (StringUtils.isNotEmpty(message)) {
+            throw new ValidationException("Domain is invalid. Because " + message);
+        }
+        if (domain.getEnableHttps().equals(Domain.EnableHttps.ON.getValue())) {


FORCE 也是开 HTTPS 的意思

backend/src/main/java/com/alibaba/higress/console/controller/RoutesController.java

backend/src/main/java/com/alibaba/higress/console/controller/dto/TlsCertificate.java

CH3CHO · 2023-05-09T01:59:47Z

@jameszhangyukun Hello, Is there any progress on this work? Could you resolve the conflicts first?

jameszhangyukun requested review from xiangxiuhui and CH3CHO as code owners February 23, 2023 09:50

CH3CHO requested changes Feb 23, 2023

View reviewed changes

jameszhangyukun force-pushed the validate branch from 3aa2f26 to 7d7666c Compare February 23, 2023 14:24

jameszhangyukun force-pushed the validate branch from 7d7666c to f653bf7 Compare February 24, 2023 06:20

CH3CHO requested changes Feb 25, 2023

View reviewed changes

jameszhangyukun force-pushed the validate branch from f653bf7 to 9d7d46d Compare February 27, 2023 05:59

CH3CHO requested changes Feb 27, 2023

View reviewed changes

feature:add validate

a077add

jameszhangyukun force-pushed the validate branch from 9d7d46d to a077add Compare March 1, 2023 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature:add validate #114

feature:add validate #114

jameszhangyukun commented Feb 23, 2023

CH3CHO Feb 23, 2023

jameszhangyukun Feb 23, 2023

CH3CHO Feb 23, 2023

jameszhangyukun Feb 23, 2023

CH3CHO Feb 23, 2023

CH3CHO Feb 23, 2023

jameszhangyukun commented Feb 23, 2023

CH3CHO Feb 25, 2023

CH3CHO Feb 25, 2023

CH3CHO Feb 27, 2023

CH3CHO commented May 9, 2023 •

edited

Loading

feature:add validate #114

Are you sure you want to change the base?

feature:add validate #114

Conversation

jameszhangyukun commented Feb 23, 2023

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameszhangyukun commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CH3CHO commented May 9, 2023 • edited Loading

CH3CHO commented May 9, 2023 •

edited

Loading