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

INT-4233: Add (S)FTP FileInfo Header (Streaming) #2065

Closed
wants to merge 6 commits into from

Conversation

garyrussell
Copy link
Contributor

JIRA: https://jira.spring.io/browse/INT-4233

Add the complete file info as JSON (when Jackson or Boon available) to the message headers
when streaming inbound.

Provide a mechanism to configure Boon to provide similar output to Jackson.

Also allow subclasses to provide their own object mapper.

Also clean up after the AMQP DSL tests, and don't use a queue foo. I often have such a queue
with content; this caused tests to fail.

@@ -132,12 +132,15 @@ public Statement apply(Statement base, Description description) {
return super.apply(base, description);
}

public void removeTestQueues() {
public void removeTestQueues(String... additionalQueues) {
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 to merge #2064 first?
We won't need this change because there won't be this local BrokerRunning any more.
The spring-rabbit-junit BrokerRunning has this method already.

Feel free to apply those AmqpTests changes during merge that PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved AMQP test fix to #2064

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

That's the first round.
I will think about the rest meanwhile...

@@ -67,6 +70,18 @@ public BoonJsonObjectMapper() {
this.objectMapper = JsonFactory.create();
}

public BoonJsonObjectMapper(Consumer<JsonParserFactory> jpfConfig, Consumer<JsonSerializerFactory> jsfConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

??? Why can't we just request JsonParserFactory and JsonSerializerFactory?

if (jsfConfig != null) {
jsfConfig.accept(jsf);
}
this.objectMapper = new ObjectMapperImpl(jpf, jsf);
Copy link
Member

Choose a reason for hiding this comment

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

Seems for me JsonFactory.create(jpf, jsf) would be better.

.setHeader(FileHeaders.REMOTE_FILE, file.getFilename());
if (getObjectMapper() != null) {
try {
builder.setHeader(FileHeaders.REMOTE_FILE_INFO, getObjectMapper().toJson(file));
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we transfer FileHeaders.REMOTE_FILE_INFO as a regular FileInfo instance if we don't want JSON and everything downstream is in memory?

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 why can't we build JSON manually from such a simple FileInfo object?..
To be honest I'm not sure that we should serialize to the JSON a target implementation returned by the getFileInfo().

@@ -74,6 +79,12 @@ protected AbstractRemoteFileStreamingMessageSource(RemoteFileTemplate<F> templat
Comparator<AbstractFileInfo<F>> comparator) {
this.remoteFileTemplate = template;
this.comparator = comparator;
if (JacksonJsonUtils.isJackson2Present()) {
this.objectMapper = JsonObjectMapperProvider.newInstanceBuilder(false).build();
Copy link
Member

Choose a reason for hiding this comment

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

So, if there is no jackson, but is boon we fail with the:

Neither jackson-databind.jar, nor boon.jar

Because of:

else if (boonPresent) {
	return new BoonJsonObjectMapperBuilder();
}
else {
	throw new IllegalStateException("Neither jackson-databind.jar, nor boon.jar is present in the classpath.");
}

Why an approach from the FileSplitter doesn't work here?

JsonObjectMapperProvider.jsonAvailable() ? JsonObjectMapperProvider.newInstance() : null;

In other words fallback to boon if present and no jackson.

*/
public static JsonObjectMapperBuilder<?> newInstanceBuilder(boolean preferBoon) {
if (JacksonJsonUtils.isJackson2Present() && (!preferBoon || !boonPresent)) {
return new JacksonJsonObjectMapperBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

I think existing newInstance() ca use these builders with all those preferences.

.setHeader(FileHeaders.REMOTE_FILE, file.getFilename());
if (getObjectMapper() != null) {
try {
builder.setHeader(FileHeaders.REMOTE_FILE_INFO, getObjectMapper().toJson(file));
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 why can't we build JSON manually from such a simple FileInfo object?..
To be honest I'm not sure that we should serialize to the JSON a target implementation returned by the getFileInfo().

The adapter puts the remote directory and file name in headers `FileHeaders.REMOTE_DIRECTORY` and `FileHeaders.REMOTE_FILE` respectively.
Starting with _version 5.0_, complete remote file information, in JSON, is provided in the `FileHeaders.REMOTE_FILE_INFO` header, if a supported JSON object mapper on the classpath - currently Jackson 2 and Boon.

Common properties are available as top level map entries, but the complete `FTPFile` object is included, under key `fileInfo`, providing all properties supported by the underlying library.
Copy link
Member

Choose a reason for hiding this comment

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

OK, I see that you explain my concerns here, but still would like to share what is in mind respectively

@garyrussell
Copy link
Contributor Author

OK; let's take that simpler approach - we can just add toJson on the abstract class.

I wasted a lot of time getting Boon to work something like Jackson's OOTB mapper and it was still not very reliable.

@garyrussell
Copy link
Contributor Author

Rejected.

@artembilan
Copy link
Member

... getting Boon to work something like Jackson's OOTB mapper and it was still not very reliable.

Yeah... That might was wrong choice in the beginning. And that's maybe why SF doesn't provide its support.

Now I'm looking into FileSplitter and not sure why we use objectMapper there. The FileMarker is so simple type to represent it as a JSON manually...

@garyrussell
Copy link
Contributor Author

Have written a simple serializer; testing now.

.setHeader(IntegrationMessageHeaderAccessor.CLOSEABLE_RESOURCE, session)
.setHeader(FileHeaders.REMOTE_DIRECTORY, file.getRemoteDirectory())
.setHeader(FileHeaders.REMOTE_FILE, file.getFilename())
.setHeader(FileHeaders.REMOTE_FILE_INFO, (file.toJson()))
Copy link
Member

Choose a reason for hiding this comment

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

So, you still aren't going to provide alternative with direct object, not JSON. Are you?

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 thought about also posting the object but decided not to. Do you think we should?

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 guess something like FileHeaders.RAW_FILE_INFO.

Copy link
Member

Choose a reason for hiding this comment

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

M-m-m. No, I thought about something like boolean fileInfoJson.
The same meaning like we have in FileSplitter.markersJson.
I understand that it is a new feature here, unless an existing the already markers.
But seems for me for consistency it won't hurt to support raw object here as well.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Very close to merge IMO...

Foo foo = new Foo();
String json = SimpleJsonSerializer.toJson(foo, "fileInfo");
@SuppressWarnings("unchecked")
Map<String, Object> fromJson = JsonObjectMapperProvider.newInstance().fromJson(json, Map.class);
Copy link
Member

Choose a reason for hiding this comment

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

Does it work for the Foo.class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the SimpleJsonSerializer is object to JSON only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh - ignore that 😄

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but if it is JSON String, the Jackson must be able to deserialize it into Foo.
I mean .fromJson(json, Foo.class)

.setHeader(IntegrationMessageHeaderAccessor.CLOSEABLE_RESOURCE, session)
.setHeader(FileHeaders.REMOTE_DIRECTORY, file.getRemoteDirectory())
.setHeader(FileHeaders.REMOTE_FILE, file.getFilename())
.setHeader(FileHeaders.REMOTE_FILE_INFO,
this.fileInfoJson ? (file.toJson()) : file.getFileInfo())
Copy link
Member

Choose a reason for hiding this comment

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

Why file.getFileInfo()?
Isn't consistent with file.toJson().
So, we are expecting the FileInfo object there, not low-level entity.

assertThat(fileInfo, containsString("remoteDirectory\":\"/foo"));
assertThat(fileInfo, containsString("permissions\":\"-rw-rw-rw"));
assertThat(fileInfo, containsString("size\":42"));
assertThat(fileInfo, containsString("directory\":false")); // Boon doesn't emit default values (false)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can not talk about Boon here any more


The adapter puts the remote directory and file name in headers `FileHeaders.REMOTE_DIRECTORY` and `FileHeaders.REMOTE_FILE` respectively.
Starting with _version 5.0_, additional remote file information, in JSON, is provided in the `FileHeaders.REMOTE_FILE_INFO` header.
If you set the `fileInfoJson` property on the `SftpStreamingMessageSource` to `false`, the `LsEntry` object provided by the underlying JSch library is used instead.
Copy link
Member

Choose a reason for hiding this comment

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

Well, I see you document your solution.
But why this way?
Why that FileInfo.getFileInfo() isn't enough to get access to the underlying entity?

Seems for me our FileInfo is more convenient in most cases, especially when we have a common process after FTP as well as SFTP.
And as I said above: it isn't consistent with JSON representation.

Any additional argument to go your way?
Thanks

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 point - changing.

@@ -95,7 +125,6 @@ public PollerMetadata defaultPoller() {
public MessageSource<InputStream> ftpMessageSource() {
FtpStreamingMessageSource messageSource = new FtpStreamingMessageSource(template(), null);
messageSource.setRemoteDirectory("ftpSource/");
messageSource.setFilter(new FtpPersistentAcceptOnceFileListFilter(new SimpleMetadataStore(), "streaming"));
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the new part of the test (with FileInfo) has some results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have to fix this when your PR is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Got it! Let's merge your one then I'll rebase my and we shell see how is it going!

JIRA: https://jira.spring.io/browse/INT-4233

Add the complete file info as JSON (when Jackson or Boon available) to the message headers
when streaming inbound.

Provide a mechanism to configure Boon to provide similar output to Jackson.

Also allow subclasses to provide their own object mapper.

Also clean up after the AMQP DSL tests, and don't use a queue `foo`. I often have such a queue
with content; this caused tests to fail.
@@ -47,4 +47,10 @@
*/
public static final String MARKER = PREFIX + "marker";

/**
* JSON representation of remote file information (if a JSON object mapper is
* available).
Copy link
Member

Choose a reason for hiding this comment

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

Out-dated JavaDoc.
Will fix on merge

@artembilan
Copy link
Member

Merged as 66b53e7

@artembilan artembilan closed this Feb 24, 2017
@garyrussell garyrussell deleted the INT-4233 branch March 17, 2017 18:13
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.

None yet

2 participants