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

server: support json object in query parameters for configurations #70

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bhufmann
Copy link
Contributor

@bhufmann bhufmann commented Sep 6, 2024

PR based on PR #66, which commits are included.

An additional commit is added:

  • server: support json object in query parameters for configurations
    This commit also includes the corresponding swagger updates.

This needs change in Trace Compass: eclipse-tracecompass/org.eclipse.tracecompass#144

Signed-off-by: Bernd Hufmann [email protected]

@bhufmann bhufmann changed the title Json schema 2 json if server: support json object in query parameters for configurations Sep 6, 2024
// Pass JSON object as json string
params.put(TmfConfiguration.JSON_STRING_KEY, queryParameters.getParameters().toString());
} else {
// Convert it to a Map<String, Object>
Copy link
Contributor Author

@bhufmann bhufmann Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can discuss the removal of the support for the query param map in the TSP to avoid having clients to implement the support for 2 ways of user input. (To be discussed). This would be done in a follow-up PR.

.path(CONFIG_INSTANCES_PATH);

Bundle bundle = Platform.getBundle("org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests");
IPath defaultPath = new org.eclipse.core.runtime.Path("config/custom-execution-analysis.json"); //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the json file name (without the path "config/") is defined at the top of the file, and passed to this function, but still a hardcoded value is used here?

Copy link
Contributor

@marcdumais-work marcdumais-work Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there is a typo in the file name, as defined at the top of the file, so the json file would not have been found if used. See comment up there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll use the constant and fix the typo.

@@ -78,6 +90,11 @@ public class ConfigurationManagerServiceTest extends RestServerTest {
private static final String EXPECTED_CONFIG_DESCRIPTION = "XML Data-driven analysis: " + VALID_NAME;
private static final String PATH_TO_INVALID_PATH = getPath(PATH_INVALID + INVALID_XML_FILE);
private static final String PATH_TO_VALID_PATH = getPath(PATH_VALID + VALID_XML_FILE);
private static final String VALID_JSON_FILENAME = "custome-execution-analysis.json";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a typo: "custome-" vs "custom-"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

See eclipse-cdt-cloud/trace-server-protocol#103

Also, add test for TmfConfigurationSourceType with schema json object.

[Added] Serialize TmfConfigurationSourceType schema as json object

Signed-off-by: Bernd Hufmann <[email protected]>
@bhufmann bhufmann force-pushed the json-schema-2-json-if branch 2 times, most recently from 3569358 to 897c200 Compare September 19, 2024 12:50
This commit also includes the corresponding swagger updates.

[Added] support json object in query parameters for configurations

Signed-off-by: Bernd Hufmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants