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

Unify approach for creating operation outcome issues across the codebase #2596

Closed
JohnTimm opened this issue Jul 9, 2021 · 1 comment
Closed

Unify approach for creating operation outcome issues across the codebase #2596

JohnTimm opened this issue Jul 9, 2021 · 1 comment
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@JohnTimm
Copy link
Collaborator

JohnTimm commented Jul 9, 2021

There are a number of different components across that codebase that generate operation outcome issues for a variety of reasons. I'd like to create a single set of utility methods for creating operation outcome issues (and operation outcome resources) and ensure that all components are using the same set of utilities. We have generally moved in this direction but things are still a bit scattered. These utility methods could took a flag that indicates whether user input is included in the message so that we could selectively encode before returning them to the client. It is a compromise between completely ad hoc approach and an "encode everything" approach.
@JohnTimm JohnTimm mentioned this issue Jul 9, 2021
Merged
@prb112 prb112 added the enhancement New feature or request label Jul 12, 2021
lmsurpre added a commit that referenced this issue Aug 3, 2021
@lmsurpre
Revert the change to FHIRResource.exceptionResponse
d80e18d 
Instead of ensuring that every string field is encoded for HTML (the
previous approach for this PR), I reverted back to the original code
which only encodes OperationOutcome.Issue.diagnostics.

The plan is to more selectively encode specific fields that are known to
contain user-provided content (#2596). Once that is done, I think we
should remove the logic that encodes all of the diagnostic strings.

Additionally, since we no longer encode the whole thing, I added
Encode.forHtml back to the buildUnsupportedResourceTypeException helpers
(but just around the user-provided portion of the error message).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Aug 3, 2021
@lmsurpre
Revert the change to FHIRResource.exceptionResponse
563dd9f 
Instead of ensuring that every string field is encoded for HTML (the
previous approach for this PR), I reverted back to the original code
which only encodes OperationOutcome.Issue.diagnostics.

The plan is to more selectively encode specific fields that are known to
contain user-provided content (#2596). Once that is done, I think we
should remove the logic that encodes all of the diagnostic strings.

Additionally, since we no longer encode the whole thing, I added
Encode.forHtml back to the buildUnsupportedResourceTypeException helpers
(but just around the user-provided portion of the error message).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Aug 17, 2021
@lmsurpre
Introduce EncodingVisitor and remove redundant resource type checking…
970633d 
… in AbstractOperation (#2594)

* Introduce EncodingVisitor and use it to encode OperationOutcome for HTML

Previously, we had logic to encode the "diagnostic" element.
However, each place that populated issues with details needed its own
encoding.
With this change, we can encode all fields in the OperationOutcome
consistently and perhaps the new visitor could be useful in some other
contexts as well.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Remove resource type checking from AbstractOperation

Now that we ensure that the resource type is valid prior to invoking the
operation, there's no need for having this check in AbstractOperation
and therefor also no need to have the PROPNAME_PATH_PARAMETER in the
OperationContext

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Revert the change to FHIRResource.exceptionResponse

Instead of ensuring that every string field is encoded for HTML (the
previous approach for this PR), I reverted back to the original code
which only encodes OperationOutcome.Issue.diagnostics.

The plan is to more selectively encode specific fields that are known to
contain user-provided content (#2596). Once that is done, I think we
should remove the logic that encodes all of the diagnostic strings.

Additionally, since we no longer encode the whole thing, I added
Encode.forHtml back to the buildUnsupportedResourceTypeException helpers
(but just around the user-provided portion of the error message).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member

lmsurpre commented Sep 20, 2021 •
edited
Loading

Team decision: use Encode.forHtml() in all the places where we're reflecting user input back in OperationOutcome issue messages. No separate utility.

@lmsurpre lmsurpre added the wontfix This will not be worked on label Sep 20, 2021
@lmsurpre lmsurpre mentioned this issue Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@prb112 @lmsurpre @JohnTimm