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.

Sign up for GitHub

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

Jump to bottom

[AERIE-1906] Resolve SerializedValue LGTM errors #238

Merged
merged 4 commits into from Jul 18, 2022
Merged

[AERIE-1906] Resolve SerializedValue LGTM errors #238

merged 4 commits into from Jul 18, 2022

Conversation

pcrosemurgy
Copy link
Contributor

@pcrosemurgy pcrosemurgy commented Jul 7, 2022 •
edited

  • Tickets addressed: AERIE-1906
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

From #220.

Resolves LGTM SerializedValue errors reported here.

Verification

All tests passing.

Documentation

None.

Future work

Resolve LGTM warnings (see https://lgtm.com/projects/g/NASA-AMMOS/aerie/alerts/?mode=tree).

@Twisol
Copy link
Contributor

Twisol commented Jul 7, 2022

Sorry... GitHub notifications never actually tell me a PR is just a draft. 🤦‍♂️

@pcrosemurgy
Copy link
Contributor Author

Sorry... GitHub notifications never actually tell me a PR is just a draft. 🤦‍♂️

No worries! All solid feedback :) Thanks

@pcrosemurgy pcrosemurgy force-pushed the refactor/aerie-1906-lgtm-serialized-value branch 2 times, most recently from e06334d to c3a15a1 Compare July 7, 2022 19:14
@pcrosemurgy pcrosemurgy mentioned this pull request Jul 7, 2022
Merged
@pcrosemurgy pcrosemurgy force-pushed the refactor/aerie-1906-lgtm-serialized-value branch from c3a15a1 to 173be82 Compare July 7, 2022 19:31
@pcrosemurgy pcrosemurgy marked this pull request as ready for review July 7, 2022 19:36
@pcrosemurgy
Copy link
Contributor Author

Opened this PR prematurely. Am investigating the e2e-tests failure.

@pcrosemurgy pcrosemurgy force-pushed the refactor/aerie-1906-lgtm-serialized-value branch from 173be82 to 480b9de Compare July 8, 2022 18:29
skovati
skovati approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@skovati skovati left a comment

Choose a reason for hiding this comment

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

LGTM! (heh) Smart move refactoring SerializedValue into an interface.

Should we create a separate ticket to capture the string escape work or were you planning on implementing that in this PR?

adrienmaillard
adrienmaillard approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@adrienmaillard adrienmaillard left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@pcrosemurgy
Copy link
Contributor Author

Should we create a separate ticket to capture the string escape work or were you planning on implementing that in this PR?

Good point! I'll check in with Adrien on this, the preexisting TODO notes about string escaping should probably be removed in place of a ticket.

@pcrosemurgy pcrosemurgy force-pushed the refactor/aerie-1906-lgtm-serialized-value branch from ce351ee to 88d926a Compare July 18, 2022 00:21
@pcrosemurgy pcrosemurgy merged commit a034e1b into develop Jul 18, 2022
@pcrosemurgy pcrosemurgy deleted the refactor/aerie-1906-lgtm-serialized-value branch July 18, 2022 02:21
camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Jul 18, 2022
@camargo
build: upgrade deps to latest
f3ff4ee 
- Capitalize  endpoint method names from sveltejs/kit#5513
- Upgrade e2e Banananation from NASA-AMMOS/aerie#238
@dyst5422
Copy link
Contributor

Why are we doing sanitization for GQL instead of using variables? That's one of the great value propositions of the GraphQL standard - built in sanitization.

@pcrosemurgy
Copy link
Contributor Author

Sure! I'll change the wording of the ticket.

@dyst5422
Copy link
Contributor

I don't think my comment is scoped to this ticket as the string formatting predates this ticket. Specifically looking at lines like

aerie/scheduler-server/src/main/java/gov/nasa/jpl/aerie/scheduler/server/services/GraphQLMerlinService.java

Lines 137 to 145 in 88d926a

"query getPlanMetadata { "
+ "plan_by_pk( id: %s ) { "
+ " id revision start_time duration "
+ " mission_model { "
+ " id name version "
+ " uploaded_file { name } "
+ " } "
+ " simulations(limit:1, order_by:{revision:desc} ) { arguments }"
+ "} }"
where we were already doing string templating instead of using variables

@pcrosemurgy
Copy link
Contributor Author

Yup! But your point 100% still stands, we should use variables. https://jira.jpl.nasa.gov/browse/AERIE-1949 should address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Twisol Twisol Twisol left review comments

@skovati skovati skovati approved these changes

@adrienmaillard adrienmaillard adrienmaillard approved these changes

@patkenneally patkenneally Awaiting requested review from patkenneally

@mattdailis mattdailis Awaiting requested review from mattdailis

@goetzrrGit goetzrrGit Awaiting requested review from goetzrrGit

@dyst5422 dyst5422 Awaiting requested review from dyst5422

@camargo camargo Awaiting requested review from camargo

@JoelCourtney JoelCourtney Awaiting requested review from JoelCourtney

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pcrosemurgy @Twisol @dyst5422 @skovati @adrienmaillard