chicks / sugarcrm

A ruby based REST Client for SugarCRM
MIT License
90 stars 64 forks source link

Unable to Upload Excel Documents #39

Closed frugardc closed 13 years ago

frugardc commented 13 years ago

Attempting to use the gem to upload a simple excel (.xls) document fails. Verified that CSV works. Looks like there is an error somewhere the gem tries to convert the xls to_json. Additionally, I have seen that it works with SOME pdf files, not with others.

require 'test_helper'
 class TestDocumentAttachment < ActiveSupport::TestCase
  context "A SugarCRM.connection" do
    should "1. Create a CSV Document" do
      csvfile = File.read("../tmp/m_accounts.csv")
      assert csvfile.to_json
      csv = SugarCRM::Document.new
      csv.active_date = Date.today,
      csv.document_name = "test_excel.xls"
      csv.filename = "test_excel.xls"
      csv.revision = 0
      csv.uploadfile = csvfile
      csv.save 
      assert !csv.new?
    end

    should "2. Create an Excel Document" do 
      excelfile = File.read("../tmp/test_excel.xls")
      assert excelfile.to_json
      excelfile = SugarCRM::Document.new
      excelfile.active_date = Date.today,
      excelfile.document_name = "test_excel.xls"
      excelfile.filename = "test_excel.xls"
      excelfile.revision = 0
      excelfile.uploadfile = excelfile
      excelfile.save 
      assert !excelfile.new?
    end
  end
end

Finished in 1.720047 seconds.

  1) Error:
test: A SugarCRM.connection should 2. Create an Excel Document. (TestDocumentAttachment):
ArgumentError: redundant UTF-8 sequence

2 tests, 2 assertions, 0 failures, 1 errors
cfrugard@cfrugard-laptop:~/projects/sugar/test$ 

frugardc commented 13 years ago

Possibly related to

https://rails.lighthouseapp.com/projects/8994/tickets/1112-redundant-utf-8-sequence-in-stringto_json

frugardc commented 13 years ago

require 'json/pure' seems to fix the issue in a vanilla irb console after requiring activesupport and json, but cant seem to get it working in the gem.

chicks commented 13 years ago

A few things:

  1. Use a different variable name for the Excel file (you have the same variable assigned to SugarCRM::Document.new as you do File.read)
  2. Make sure you are Base64 encoding the file before you assign it to the SugarCRM::Document record. Something like:

    excelfile.uploadfile = Base64.encode64(File.read("../tmp/test_excel.xls"))

frugardc commented 13 years ago

see what you mean on the test. i just did a quick one, and it wasnt making it that far anyway. Your suggestion allows the file to upload, but it becomes unrecognizable when you try to view from Sugar. Is it that the file also needs to then be decoded on the other side?

Like I said earlier, using require 'json/pure' in irb seems to allow the xls file to be converted directly to_json without error.

chicks commented 13 years ago

Sugar expects any file upload to be base64 encoded. I've never tried to upload a file on the Document object itself. I'm not sure it even works :/

I do know this works:

should "Update a document revision" do
  file = File.read("test/config_test.yaml")
  # Create a new document, no file is attached or uploaded at this stage.
  d = SugarCRM::Document.create(
    :revision     => 1, 
    :active_date  => Date.today,
    :filename     => "config_test.yaml", 
    :document_name=> "config_test.yaml",
    :uploadfile   => "config_test.yaml"
  )
  # Did we succeed?
  assert !d.new?
  # Create a new document revision, attaching the file to the document_revision, and associating the document
  # revision with parent document
  assert SugarCRM.connection.set_document_revision(d.id, d.revision + 1, {:file => file, :file_name => "config_test.yaml"})
  # Delete the document
  assert d.delete
  # Remove any document revisions associated with that document
  SugarCRM::DocumentRevision.find_all_by_document_id(d.id).each do |dr|
    assert dr.delete
  end
end
frugardc commented 13 years ago

It actually doesn't. I was just showing the error on the read of the file. Can you try that test with an excel file?

frugardc commented 13 years ago

What happens, I think is...

to_json is called on the file itself, resulting in "ArgumentError: redundant UTF-8 sequence" when this file is certain binary files.

which is a bug in ActiveSupport, as seen here...

https://rails.lighthouseapp.com/projects/8994/tickets/1112-redundant-utf-8-sequence-in-stringto_json

I found a workaround to this by using to_json from json/pure, rather than to_json from activesupport. No idea why or how, but I verified that in irb.

chicks commented 13 years ago

Yeah, we ran into issues with the active_support JSON implementation. We took it out in 0.9.10.

davidsulc commented 13 years ago

The gem actually uses the json gem (not ActiveSupport) for json parsing. What happens if you require json/ext instead of json/pure? The bug might be in the native implementation (as opposed to the Ruby implementation) of the josn gem.

frugardc commented 13 years ago

It works with both json/ext and json/pure, but as soon as I require activesupport, it bombs with ArgumentError: redundant UTF-8 sequence

Thats the error I get in my rails project. I am using the sugarcrm gem in a Rails project. Is it possible that the sugarcrm gem loads before rails, so that ActiveResource's to_json gets called instead of the JSON version? That's what seems to be happening, judging by the error I am seeing.

chicks commented 13 years ago

What happens if you change the ActiveSupport JSON implementation a la:

https://github.com/rails/rails/commit/3c4c6bd0df598f865f49a983b4c65c415af4bcfc

davidsulc commented 13 years ago

Try putting a ruby file in your initializers folder (in your rails app) with require 'json'. Does your app then work correctly?

My hunch is that, as you also suspect, it has to do with loading order...

frugardc commented 13 years ago

neither seems to do the trick

chicks commented 13 years ago

Do you have a simple rails project we can debug off of?

davidsulc commented 13 years ago

Yes, a basic implementation (i.e. demonstrating your issue, without any other functionality) would help immensely.

If you can't post your app, could you (e.g.) for the one here: https://github.com/davidsulc/sugar_on_rails_basic and add the functionality you're having issues with? (Commit https://github.com/davidsulc/sugar_on_rails_basic/commit/17ecc9c3a089e6c5828414d522772648e0bda39c should get you started easily enough.)

frugardc commented 13 years ago

How's this?

https://github.com/frugardc/sugar_on_rails_basic

frugardc commented 13 years ago

added a test to test/unit and a few test files to tmp

davidsulc commented 13 years ago

I had to modify the tests to get them to load when calling rake (I've push the modified rails test app here: https://github.com/davidsulc/frugardc_rails ). Also, they originally didn't actually test the file upload. You've both been added as contributors to the repository, FYI.

I also had to add commit https://github.com/chicks/sugarcrm/commit/a4e73319ebfe5d4670f13af0ddd407c815ec6dcf to get the CSV test case to work.

I can confirm the issue with the test case as it currently is. Haven't yet been able to look into it much, but adding require 'json/pure' in config/initializers/sugarcrm.rb causes an error...

frugardc commented 13 years ago

You were able to see the failure of the excel test though, correct?

davidsulc commented 13 years ago

Absolutely. I kind of got sidetracked after that (among which writing upcoming articles on using the gem both with and without Rails...).

However, I was unable to reproduce your fix requiring 'json/pure'. Could you provide some instructions on how you did that so I can look into it?

frugardc commented 13 years ago

I was only able to fix it using json/pure in a straight IRB session.

irb 

require 'rubygems'
require 'activeresource'

file.to_json = ArgumentError: redundant UTF-8 sequence

require 'json/pure'

file.to_json = Success
frugardc commented 13 years ago

Just checking in. Were you able to reproduce the above results?

davidsulc commented 13 years ago

It seems that in an irb session, by requiring in the order you've given, it sometimes works: some binary files work, others don't...

That said, according to http://stackoverflow.com/questions/683989/how-do-you-deal-with-the-conflict-between-activesupportjson-and-the-json-gem it's best not to rely on the to_json method to avoid conflicts.

And if I call JSON.generate(excel_file) (where excel_file is the binary contents of the Excel file), I get an ArgumentError: invalid byte sequence in UTF-8 coming from the JSON gem (not activeSupport).

In addition, even if I specify the JSON backend that ActiveSupport uses to encode JSON, the tests still don't work.

Maybe chicks will have a better idea, but at the moment I don't see how to solve this issue...

chicks commented 13 years ago

I'll try and take a look next week. This week will be pretty nasty w/ prepping for SugarCon, and I already owe David Sulc a bunch of feedback on some articles he wrote. Things have been absolutely INSANE around the office...

I just wish I could go back to coding :/

davidsulc commented 13 years ago

Heh. I don't know how I failed to realize this until now, but you need to encode the file in base 64 before setting the uploadfile value. See https://github.com/davidsulc/frugardc_rails/commit/fda15bc5c8abd781a812836ee005befac47aebbf

Frugardc, can you confirm this solves the issue?

davidsulc commented 13 years ago

Frugardc, I'll close this issue as resolved. Fell free to reopen if you're still having issues.

For reference, here is an example of document upload:

excelfile = File.read(File.join(File.dirname(__FILE__),"test_excel.xls"))
exceldoc = SugarCRM::Document.new
exceldoc.active_date = Date.today
exceldoc.document_name = "test_excel.xls"
exceldoc.filename = "test_excel.xls"
exceldoc.revision = 0
exceldoc.uploadfile = Base64.encode64(excelfile)
exceldoc.save
frugardc commented 13 years ago

That was something that was tried earlier on. Encoding64 enabled the upload of the file, but when you try to read the file out of sugar, it is completely unreadable. Can you see if you have the same behavior?

davidsulc commented 13 years ago

Ah, I misunderstood your problem.

I've opened a new issue here: https://github.com/chicks/sugarcrm/issues/41