views:

242

answers:

2

I am pretty sure this code is fine. I wanted to know what you guys think about the insertMediaTags function (2nd func). The things i am worried about is the below concurrent safe? and if insertMediaTags is optimize enough? note it is in a transaction due to first func but it is also in a loop which could mean its bad?

I am open to any coding practice, style or suggestions you guys may have. (I know someone will ask, i am using sqlite ATM but its prototype code to use with mysql or a version of ms sql or another)

    {
        long mediaId;
        //all the credentials should be verified by this point.
        command.CommandText = "SELECT mediaId FROM media " +
            "WHERE userId=@userId AND title=@title;";
        command.Parameters.Add("@userId", DbType.Int64).Value = m.userid;
        command.Parameters.Add("@title", DbType.String).Value = m.title;
        if (command.ExecuteScalar() != null)
            throw new System.InvalidOperationException("Title already exisit");

        using (var dbTrans = connection.BeginTransaction())
        {
            command.CommandText =
                "INSERT INTO " +
                "media ( userid,  catagory,  creation_date,  current_media_date,  current_desc_date,  licence,  title,  desc,  ext) " +
                "VALUES(@userid, @catagory, @creation_date, @current_media_date, @current_desc_date, @licence, @title, @desc, @ext); " +
                "SELECT last_insert_rowid() AS RecordID;";

            DateTime currentDate = m.creation_date;
            command.Parameters.Add("@userid", DbType.Int64).Value = m.userid;
            command.Parameters.Add("@catagory", DbType.Int64).Value = m.catagory;
            command.Parameters.Add("@creation_date", DbType.DateTime).Value = m.creation_date;
            command.Parameters.Add("@current_media_date", DbType.DateTime).Value = currentDate;
            command.Parameters.Add("@current_desc_date", DbType.DateTime).Value = currentDate;
            command.Parameters.Add("@licence", DbType.Int64).Value = m.license;
            command.Parameters.Add("@title", DbType.String).Value = m.title;
            command.Parameters.Add("@desc", DbType.String).Value = m.desc;
            command.Parameters.Add("@ext", DbType.Int64).Value = m.ext;

            mediaId = (long)command.ExecuteScalar();
            //m.collaborateWith
            insertInspired(inspireLinks.external, inspireLinks.internalPair, mediaId);
            insertDerived(deriveLinks.external, deriveLinks.internalPair, mediaId);
            insertMediaTags(m.listTagString, mediaId);
            //command.CommandText = "END TRANSACTION;";            command.ExecuteNonQuery();

            updateMediaForWatchers(m.userid, mediaId, m.catagory, currentDate);
            dbTrans.Commit();
        }
        return mediaId;
    }

    void insertMediaTags(List<string> tags, long mediaId)
    {
        foreach(string tag in tags)
        {
            //assure tag exist
            long tagId;
            command.CommandText = "SELECT tagid FROM tag_name WHERE title=@title;";
            command.Parameters.Add("@title", DbType.String).Value = tag;
            object o = command.ExecuteScalar();
            if (o == null)
            {
                command.CommandText =
                    "INSERT INTO tag_name(title) VALUES(@title); " +
                    "SELECT last_insert_rowid() AS RecordID;";
                command.Parameters.Add("@title", DbType.String).Value = tag;
                tagId = (long)command.ExecuteScalar();
            }
            else
                tagId = (long)o;

            command.CommandText =
                "INSERT INTO media_tags(mediaId, tagid) " +
                "VALUES(@mediaId, @tagid);";
            command.Parameters.Add("@mediaId", DbType.Int64).Value = mediaId;
            command.Parameters.Add("@tagid", DbType.Int64).Value = tagId;
            command.ExecuteNonQuery();

            command.CommandText =
                "UPDATE tag_name SET count = count+1 "+
                "WHERE tagid=@tagid";
            command.Parameters.Add("@tagid", DbType.Int64).Value = tagId;
            command.ExecuteNonQuery();
        }
    }
+3  A: 

No it's not concurrent safe. You have a potential race condition between the SELECT to determine whether the tag exists, and the INSERT to create the tag if it does not. Imagine thread A does a SELECT and finds it does not exist, and then thread B does the same before thread A does the INSERT. Thread B will attempt the insert as well and fail.

Bernhard Hofmann
Beat me to it ;)
Joel Coehoorn
Put another way: the SELECT query needs to be in the transaction will the insert statements.
Joel Coehoorn
insertMediaTags is only called in the first method; which has a transaction around it. Is this still a problem? (I put comments in insertMediaTags which i removed saying it should always be called in a transaction) – acidzombie24 0 secs ago [delete this comment]
acidzombie24
Second question is how to rewrite that piece of code. I tried INSERT OR IGNORE but then could not get a suitable tagId (when it has already been insert. last_insert_rowid is not updated on ignore)
acidzombie24
You *could* use the "IF NOT EXIST (SELECT...) INSERT"
Bernhard Hofmann
A: 

In SQL Server, it's better to use the SCOPE_IDENTITY() function. Other than that I don't see a problem.

ristonj