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

XML: Add a contextual version of the parser for XML #2935

Merged
merged 7 commits into from
Dec 2, 2022

Conversation

jcazevedo
Copy link
Contributor

@jcazevedo jcazevedo commented Oct 28, 2022

This pull request adds a contextual version of the parser provided by XmlParsing.

API-wise, I was not entirely sure how to set this up, since different modules seem to follow different conventions. For example, the Cassandra module provides a withContext method directly in the CassandraFlow object (https://github.com/akka/alpakka/blob/4f77c8b6b7e443acb39fa3164613c19a626e530f/cassandra/src/main/scala/akka/stream/alpakka/cassandra/scaladsl/CassandraFlow.scala), but the AMQP module provides its own AmqpFlowWithContext object with contextual variants (https://github.com/akka/alpakka/blob/4f77c8b6b7e443acb39fa3164613c19a626e530f/amqp/src/main/scala/akka/stream/alpakka/amqp/scaladsl/AmqpFlowWithContext.scala).

@lightbend-cla-validator

Hi @jcazevedo,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

https://www.lightbend.com/contribute/cla

val in: Inlet[ByteString] = Inlet("XMLParser.in")
val out: Outlet[ParseEvent] = Outlet("XMLParser.out")
override val shape: FlowShape[ByteString, ParseEvent] = FlowShape(in, out)
@InternalApi private[xml] class StreamingXmlParser[Ctx](ignoreInvalidChars: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep the non-context version around separately to not burden it with unneeded tuple creation. But maybe that would become too much duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we can avoid some duplication by letting callers of the internal API decide how elements in the flow are created, at the expense of complicating the internal API (d66f71c). What do you think?

Attempts to avoid the unnecessary creation of tuples by parameterizing
the internal API and letting callers decide how elements in the flow are
built.
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Yes, that's better.

Comment on lines 47 to 49
getByteString = identity,
getContext = _ => (),
buildOutput = (parseEvent, _) => parseEvent)
Copy link
Member

Choose a reason for hiding this comment

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

How about putting the three fixed transform methods into a class with two objects as implementations?

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 idea. I did that in 2d980fb.

configureFactory,
getByteString = _._1,
getContext = _._2,
buildOutput = (pe, ctx) => (pe, ctx))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buildOutput = (pe, ctx) => (pe, ctx))
buildOutput = identity)

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Close now.
A few more suggestions.

@jcazevedo
Copy link
Contributor Author

@ennru: Thank you for your review! I have applied all of your suggestions.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru merged commit b813e7f into akka:master Dec 2, 2022
@jcazevedo jcazevedo deleted the xml-parser-with-context branch December 2, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants