Skip to content
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

feat(jans-config-api): improvements to existing agama endpoints and create PUT for code only #1982

Closed
jgomer2001 opened this issue Aug 1, 2022 · 5 comments · Fixed by #2015, #2028, #2046 or #2058
Assignees
Labels
comp-jans-config-api Component affected by issue or PR kind-feature Issue or PR is a new feature request

Comments

@jgomer2001
Copy link
Contributor

jgomer2001 commented Aug 1, 2022

Retrieval

Currently GETting all flows (/jans-config-api/api/v1/agama) returns all attributes; it should return fewer as mentioned here. Also, let's make single flow retrieval behave the same, ie. not outputing all attributes.

In some cases, one may like to have source in the output as well. Let's include it if a given request parameter is present, like /jans-config-api/api/v1/agama?includeSource. Applies for both GET endpoints

@mo-auto mo-auto added comp-jans-config-api Component affected by issue or PR kind-feature Issue or PR is a new feature request labels Aug 1, 2022
@jgomer2001
Copy link
Contributor Author

Creation

  1. POST to /jans-config-api/api/v1/agama has a typo when there is an unexpected attribute:

{"code":"400","message":"Value of these feilds should be null -> (transHash)."}

should be fields. The same goes for PUT

  1. POST to /jans-config-api/api/v1/agama/{qname} returns the error as a string in case of malformed source file, like this:

{"code":"400","message":"{\"error\":\"mismatched input 'hi' expecting 'Flow'\",\"symbol\":\"[@0,0:2='mua',<45>,1:0]\",\"line\":1,\"column\":0,\"message\":\"Syntax error: mismatched input 'mua' expecting 'Flow'\\nSymbol: [@0,0:2='mua',<45>,1:0]\\nLine: 1\\nColumn: 1\"}"}

but it should contain the SyntaxException as straight json, e.g:

{
    ...
    "error":"mismatched input 'hi' expecting 'Flow'",
    "symbol":"[@0,0:2='mua',<45>,1:0]",
    "line":1,
    "column":0,
    "message": "..."
}
  1. POST to /jans-config-api/api/v1/agama/{qname} returns the full json representation of the flow created. It should return an empty response or only the fields used for retrieval of single flow

  2. Small request for flow creation: can you set timestamp inside flow metadata equal to System.currentTimemillis please?

@jgomer2001
Copy link
Contributor Author

jgomer2001 commented Aug 1, 2022

Modification

  1. An endpoint is missing. We need to be able to update a flow passing the source code only - no json - as mentioned here. Can you add a new PUT handler that consumes text/plain ?

  2. Regarding the existing PUT endpoint, I couldn't make it work. Locally I have an existing flow with qname re.move

Sending this payload: { "qname": "re.move" }

gives {"code":"400","message":"Required feilds missing -> (source). "} , however source is not required for an update.

Using this payload:

{
"qname": "re.move",
"source": "Flow re.move\n\tBasepath \"remove\"\nFinish true"
}

throws internal server error. Here are the logs:

configapi.log.txt

@pujavs
Copy link
Contributor

pujavs commented Aug 4, 2022

Testing: Feedback implementation testing evidence
Agama_endpoint_improvement_test_issue#1982.xlsx

Note for:

  • includeSource feature in GET method need to pass includeSource value as true - > jans-config-api/api/v1/agama?includeSource=true
  • PUT with source in request body the url is -> /jans-config-api/api/v1/agama/source/{qname} to distinguish it from existing PUT method.
  • PATCH endpoint implemented

@jgomer2001
Copy link
Contributor Author

jgomer2001 commented Aug 5, 2022

@pujavs

  1. Regarding PUT /jans-config-api/api/v1/agama/source/{qname}

Increasing jansRevision by 1 at every call here was a good decision, thanks. However you are also updating metadata/timestamp. This is a creation timestamp so there is no need to alter this property

If the flow has agFlowTrans and/or jansCustomMessage attributes populated in ldap already, they are wiped after calling this operation - not the case of jansScrError though. These three attributes are populated (and managed) internally by the engine; no tool should touch them.

Desired behavior is to alter source and jansRevision only

  1. Regarding PATCH

Every time this endpoint is called, revision and timestamp are refreshed automatically. This is not desired. If the caller wants to explicitly supply values for them, they can be updated.

Some properties are lost as in the previous case (PUT)

Desired behavior is to alter only the data sent in the patch

  1. About runSyntaxCheck invocation

can you call setStackTrace​(empty array) on the exception caught?. Sometimes, the whole stacktrace is serialized to json and it adds a lot of clutter

@jgomer2001 jgomer2001 reopened this Aug 5, 2022
@pujavs
Copy link
Contributor

pujavs commented Aug 8, 2022

@jgomer2001

agFlowTrans and/or jansCustomMessage attributes are not returned by the GET method as these are not part of Flow schema.

PUT: input is Flow object and directly updates the Flow object as received. Even though i manually check and exclude these attributes they should be retuned by the FETCH.
PATCH: Same goes for PATCH. The existing Flow data is fetched from DB and only the attributed as received in JsonPatch operation is patched.
Kindly advice how to fetch the values of these fields which are not part of Flow.java

DB schema: error when adding agFlowTransandjansCustomMessage` attributes.
image
image
image

GET : does not return these agFlowTrans and jansCustomMessage attributes
image

jgomer2001 added a commit that referenced this issue Aug 9, 2022
jgomer2001 added a commit that referenced this issue Aug 9, 2022
jgomer2001 added a commit that referenced this issue Aug 9, 2022
yuriyz pushed a commit that referenced this issue Aug 9, 2022
)

* fix: avoid data loss #1982

* docs: remove PUT json endpoint #1982

* chore: remove unnecessary check #1982
pujavs pushed a commit that referenced this issue Aug 9, 2022
)

* fix: avoid data loss #1982

* docs: remove PUT json endpoint #1982

* chore: remove unnecessary check #1982
ossdhaval pushed a commit that referenced this issue Aug 16, 2022
)

* fix: avoid data loss #1982

* docs: remove PUT json endpoint #1982

* chore: remove unnecessary check #1982
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment