cfsimplicity / spreadsheet-cfml

Standalone library for working with spreadsheets and CSV in CFML
MIT License
126 stars 35 forks source link

The function [close] doesn't exist in the string #311

Closed Daemach closed 1 year ago

Daemach commented 1 year ago

We're suddenly having problems with csv files. The same file saved as an xls/xlsx works properly. Oddly, it affects only Linux, in this case, the Ortus docker container. The files import fine on my windows workstation. It has been working fine for months, and these containers are built from the ground up every time we push new code. I don't know what has changed.

Here's a stack trace:

image

cfsimplicity commented 1 year ago

Version 3.7.0 did include some CSV improvements which seem to be causing that exception according to your trace. The library's tests all passed though and like you I can't replicate it locally. I even spun up a vanilla Ortus docker container but that worked fine for me as well, as did an Alpine Linux variation. I'm not a docker expert, but what exact image tag are you using and are you customising it in any way?

The error seems to suggest that the java.io.fileReader object is a string, but I don't understand how that can be.

Anyway, I went back to the Commons CSV docs and it seems that closing that object isn't actually necessary since I'm closing the parser as well. So I've removed that operation and published it as v3.7.1. Please give that a go and let me know the result.

Daemach commented 1 year ago

Thanks, Julian. Below is my dockerfile. We ran into a container bug that forced me to drop back to the base commandbox image and force a specific version of Lucee.

FROM ortussolutions/commandbox

ENV BOX_SERVER_APP_CFENGINE=lucee@5.3.9+166

COPY ./ $APP_DIR
RUN rm -rf build
RUN box install --production
RUN ${BUILD_DIR}/util/warmup-server.sh
cfsimplicity commented 1 year ago

Can't see anything unusual about your dockerfile. I'm running Lucee 5.3.9.166 as well.

Let me know if 3.7.1 fixes the issue and open it up again if not.

Daemach commented 1 year ago

I just tried it, and no joy still. What is a good version number before these changes? I'll try rolling back for now.

cfsimplicity commented 1 year ago

3.6.1 was the previous release.

But I'd prefer to sort out the current version if possible. Can you post the stack trace you're now getting from 3.7.1?

Daemach commented 1 year ago

I hear that.

There is no good place to test it now - I need to get a docker staging server set up. I can try to test it later, but we're in production, so I need to get this working.

cfsimplicity commented 1 year ago

Understood, but if you've still got the error logs from having tried 3.7.1 in prod that would be great so I can see what triggered it having removed that extra close() method.

Otherwise, we'll leave this open so you can come back to it when you can.

Daemach commented 1 year ago

the problem is that our containers are load balanced, and I can only get to them using docker-exec. But I connected to all three servers and was able to get a stack trace which shows a different issue now which may be on my end. I'm looking at it.

spreadsheet-cfml should be able to read from a lucee virtual file system right? ram://temp.csv?

cfsimplicity commented 1 year ago

spreadsheet-cfml should be able to read from a lucee virtual file system right? ram://temp.csv?

Ah no, because file paths must be accessible by java.io methods, and VFS is a CFML-only concept.

To verify, I just tried and I'm getting ram:\test.csv (The filename, directory name, or volume label syntax is incorrect) from a file in the VFS which FileRead() has no problem with.

To get round it, use FileRead() and then pass the result into csvToQuery() as a string.

Daemach commented 1 year ago

Oddly this has been working fine until now... But I'll try that.

cfsimplicity commented 1 year ago

Apologies, you're right! I've just tested with an old version and VFS works. So this is a regression which I'll try and sort out.

cfsimplicity commented 1 year ago

OK I see now: previously if you specified a filepath, the library used CFML FileRead() to convert it to a string and then proceed as if you'd passed a string. But the performance improvement was to avoid that intermediate step by using the native Commons CSV java methods for parsing files directly.

Daemach commented 1 year ago

Gotcha. This may have ramifications. I’m moving everything to CBFS soon (https://github.com/coldbox-modules/cbfs). Might make a good coldbox.cfc module setting, but adding documentation would be good.

cfsimplicity commented 1 year ago

For this method I'll just swap file reading back to CFML from java so your existing code will work again. It was only part of the improvement and the main benefit comes from more efficient iteration.

I'll need to review other methods that make direct use of java IO though and as you say maybe add warnings in the docs if it's not easy to deal with.

cfsimplicity commented 1 year ago

OK, 3.7.2 is live on Forgebox with the regression fix.

Daemach commented 1 year ago

Sounds good. I think we may always have to use the file read method, as we may be moving more data to S3 too.

cfsimplicity commented 1 year ago

With 3.7.2 you won't need to for csvToQuery(): it uses FileRead() internally again.

Daemach commented 1 year ago

Yes, but that may not work with CBFS - it has its own read/write methods which probably use fileRead() under the hood for local drives, but a different method for S3/FTP, etc.