Parsing a JSON file upload

Question

I'm trying to parse a JSON file upload. I know the format JSON will arrive as this:

{ "list": [{ "atttibute1": "value1", "atttibute2": "valu2", "atttibute3": "valu3", }, { "atttibute1": "value4", "atttibute2": "value5", "atttibute3": "value6", }, { "atttibute1": "value7", "atttibute2": "value8", "atttibute3": "value9", }] } 

I developed code to create a table for this JSON file when uploading based on that format. When the user sends it, a REST request will upload the file and then call this parser.

Are there ways to enhance the code or make it more readable?

public class JSONParserFile { /** * save JSON file in database * @param file * @param tableName * @throws Exception */ public void saveFile(MultipartFile file,String tableName) throws Exception { // get connection Connection dbConnection = DB_Connection.getConnection(); StringBuilder sbCreateTable = new StringBuilder( 1024 ); JSONParserFile jsonParser=new JSONParserFile(); List<String> listColumns=jsonParser.getTableColumns(file); //get size columns int sizeColumns=listColumns.size(); // check if colunms not emptm then append statement if (!listColumns.isEmpty() ) { sbCreateTable.append( "Create table " ).append( tableName.replaceAll(" ", "_").replaceAll("\'", "") ).append( " ( " ); } //for loop colunms for ( int i = 0; i < listColumns.size(); i ++ ) { sbCreateTable.append(listColumns.get(i).replaceAll(" ", "_")); sbCreateTable.append(" VARCHAR(255)"); if(listColumns.size()-1!=i) { sbCreateTable.append(","); } } if ( listColumns.size() > 0 ) { sbCreateTable.append(" )"); } //create table jsonParser.createTable(sbCreateTable,dbConnection); //insert records in table jsonParser.getRecordsInsert(dbConnection,file,sizeColumns,tableName); } /** * get table columns from json Object * @param file * @return */ List<String> getTableColumns(MultipartFile file){ List<String> columns=new ArrayList<String>(); JSONParser parser = new JSONParser(); //parse json objet file JSONObject obj; try { //parse JSON object obj = (JSONObject )parser.parse(new InputStreamReader(file.getInputStream(), "UTF-8")); //get keys object Set<String> keys = obj.keySet(); //get first keys String keytList =keys.iterator().next().toString(); //return list of objects JSONArray array=(JSONArray)obj.get(keytList); //first object JSONObject tempObject =(JSONObject)array.get(0); //GET ATTRIBUTES Set<String> coulmns = tempObject.keySet(); for (String attribute : coulmns) { columns.add(attribute); } } catch (UnsupportedEncodingException e) { // TODO Auto-generated catch block e.printStackTrace(); } catch (IOException e) { // TODO Auto-generated catch block e.printStackTrace(); } catch (ParseException e) { // TODO Auto-generated catch block e.printStackTrace(); } return columns; } /** * * create table based file * @param sb * @param dbConnection */ public void createTable(StringBuilder sb,Connection dbConnection){ PreparedStatement preparedStatement = null; try { preparedStatement = dbConnection.prepareStatement(sb.toString()); preparedStatement.execute(); } catch (SQLException e) { // TODO Auto-generated catch block e.printStackTrace(); } } /** * insert records excel sheeet in tables * @param dbConnection * @throws Exception */ void getRecordsInsert(Connection dbConnection,MultipartFile file,int sizeColumns ,String tableName) throws Exception{ JSONParser parser = new JSONParser(); StringBuilder sbInsert = new StringBuilder( 1024 ); PreparedStatement preparedStatement = null; //parse json objet file JSONObject obj; try { obj = (JSONObject )parser.parse(new InputStreamReader(file.getInputStream(), "UTF-8")); //get keys object Set<String> keys = obj.keySet(); //get first keys String keytList =keys.iterator().next().toString(); //return list of objects JSONArray array=(JSONArray)obj.get(keytList); for (Object object : array) { //first object JSONObject tempObject =(JSONObject)object; //rest sbInsert.setLength(0); sbInsert.append("insert into "+tableName.trim().replaceAll(" ", "_")+" values("); int currentCellLenght=0; //GET ATTRIBUTES Set<String> coulmns = tempObject.keySet(); for (String attribute : coulmns) { sbInsert.append("'"+tempObject.get(attribute).toString().replaceAll("\'", "")+"'"); currentCellLenght++; //exit when reach last coulumns if(currentCellLenght==sizeColumns) { break; } //add insert rows if(currentCellLenght!=sizeColumns) { sbInsert.append(","); } } sbInsert.append(")"); preparedStatement = dbConnection.prepareStatement(sbInsert.toString()); preparedStatement.execute(); } } catch (UnsupportedEncodingException e) { // TODO Auto-generated catch block e.printStackTrace(); } catch (IOException e) { // TODO Auto-generated catch block e.printStackTrace(); } catch (ParseException e) { // TODO Auto-generated catch block e.printStackTrace(); } } } 

Answer 1

Here are my comments, in order of severity:

Bugs

(or bug-like behavior)

1. Resource leaks:

   You do not close any of the IO resources that you use (DB connection and various InputStreams you open on the input file). Unclosed resources (aka leaks) leave JVM- or OS- level open handles behind. Even if you use a DB connection pool (I hope you did, did you?), you have to close the connection to return it to the pool. Use Java 7 try-with-resources feature to optimally handle the life cycle of IO resources.

   Note: PreparedStatement is also a closeable resource.

2. SQL injection vulnerability:

   When you insert data with SQL INSERT statement, do not embed the values in the statement. Use bind variables.

3. Redundant instance creation:

   The third statement in saveFile() creates an instance of JSONParserFile. Since the method is not static, you already have an instance of JSONParserFile: this. use it.

Performance

1. Parsing the input file:

   You parse the input file twice (and create two unclosed InputStreams). 'nough said.

2. Redundant size calculation:

   If you already took the trouble to calculate listColumns.size() and put it in an int variable sizeColumns, why do you keep calling the size() method in further processing?

Design

1. Extensibility:

   When you decide that saveFile() accepts MultipartFile argument, you limit the usefulness of the method to very specific input type. However, the processing of saveFile() does not rely on any specific feature of MultipartFile and can operate on any InputStream. InputStream is more general purpose interface than MultipartFile so you will be able to use the same saveFile() for other scenarios.

   If you want to save the client the trouble of converting MultipartFile to InputStream you can offer an overloaded variation of the method:

    public void saveFile(MultipartFile file,String tableName) throws Exception { // here we are responsible for the life-cycle of the input stream try (InputStream is = file.getInputStream()) { saveFile(is, tableName); } } public void saveFile(InputStream file, String tableName) throws Exception { // do work } 

2. Modularity:

   The JSONParserFile does all the work: the IO operation of reading the input file, parsing the JSON contents, and all the SQL processing. This means that any changes in the input or output will result in changes to this monolithic class. This creates the potential problem of side effects: when you replace the JSON with XML, you may introduce bugs in the SQL processing. For the sake of modularity (and also clarity), you better break the monolith class into smaller classes that take care of one aspect of the problem domain.

Readability

1. StringBuilder:

   The use of this class (may) have performance gains, but you pay for that in readability, big time (IMO). You should also know that the java compiler replaces plus sign string concatenation with StringBuilder.append() in the bytecode. Only in very specific scenarios (frequent loops and such) does it actually make a different to explicitly use StringBuilder. I would say that, as a rule of thumb, string concatenation should be done with plus sign unless you know you have a loop that iterates over thousands of items or is called hundreds of times.

2. But that is not enough.

   Even if we replace StringBuilder with plus sign string concatenation, this statement:

   "Create table " + tableName.replaceAll(" ", "_").replaceAll("\'", "") + " ( "; 

   remains too obscure because it is hard to see where the plus sign is among all the other punctuation symbols. One way to solve this is by breaking the line according to the plus signs. However, if we look at the "big picture", what you have here is a template of an SQL statement where you want to embed table and column names. Embedding values into a String is the expertise of printf style formatting:

   String.format("Create table %s ( ", tableName.replaceAll(" ", "_").replaceAll("\'", "")); 

   This is much better. You might consider breaking the above statement to two lines for further clarity.

3. String joining:

   Java 8 stream API can (and should) be used in almost all the places where a for loop used to be. This is especially true with String-joining loops. For example, the entire loop that is used in the building of the CREATE SQL statement can be replaced by one line:

   listColumns.stream().collect(Collectors.joining(" VARCHAR(255),", "", " VARCHAR(255))")) 

   You can argue about the readability gain. someone who is familiar with java 8 stream API will know what this is about immediately.

Naming Convention

1. void getRecordsInsert()

   A getter returning void?

Answer 2

You have two options -

Send a MultipartFile along with JSON data

public void uploadFile(@RequestParam("identifier") String identifier, @RequestParam("file") MultipartFile file){ } 

OR

Send JSON data inside a MultipartFile and then parse Multipart file as mentioned below and thats it.

public void uploadFile(@RequestParam("file") MultipartFile file){ POJO p = new ObjectMapper().readValue(file.getBytes(), POJO.class); }