ronantakizawa/codereview-bench huggingface dataset:
Repository: labstack/echo
File: middleware/basic_auth_test.go (Go)

Code before:
	"bytes"
	"encoding/base64"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/assert"
)

func TestBasicAuth(t *testing.T) {
	e := echo.New()
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	res := httptest.NewRecorder()
	c := e.NewContext(req, res)
	f := func(u, p string, c echo.Context) (bool, error) {
		if u == "joe" && p == "secret" {
			return true, nil
		}
		return false, nil
	}
	h := BasicAuth(f)(func(c echo.Context) error {
		return c.String(http.StatusOK, "test")
	})
	s := func(c echo.Context) bool {
		auth := c.Request().Header.Get(echo.HeaderAuthorization)
		if strings.HasPrefix(auth, basic) {
			decoded, err := base64.StdEncoding.DecodeString(auth[len(basic)+1:])
			if err != nil {
				return false
			}
			return bytes.Equal(decoded, []byte("joe:skip"))
		}
		return false
	}

	// Valid credentials
	auth := basic + " " + base64.StdEncoding.EncodeToString([]byte("joe:secret"))
	req.Header.Set(echo.HeaderAuthorization, auth)
	assert.NoError(t, h(c))

	h = BasicAuthWithConfig(BasicAuthConfig{
		Skipper:   s,
		Validator: f,
		Realm:     "someRealm",
	})(func(c echo.Context) error {
		return c.String(http.StatusOK, "test")
	})

	// Skipped Request

Review comment (suggestion):
Tests do not need to be that complex. You could add skipper that always skips (`return true`) and verify that it skips. 

Code after:
	"encoding/base64"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/assert"
)

func TestBasicAuth(t *testing.T) {
	e := echo.New()
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	res := httptest.NewRecorder()
	c := e.NewContext(req, res)
	f := func(u, p string, c echo.Context) (bool, error) {
		if u == "joe" && p == "secret" {
			return true, nil
		}
		return false, nil
	}
	h := BasicAuth(f)(func(c echo.Context) error {
		return c.String(http.StatusOK, "test")
	})

	// Valid credentials
	auth := basic + " " + base64.StdEncoding.EncodeToString([]byte("joe:secret"))
	req.Header.Set(echo.HeaderAuthorization, auth)
	assert.NoError(t, h(c))

	h = BasicAuthWithConfig(BasicAuthConfig{
		Validator: f,
		Realm:     "someRealm",
	})(func(c echo.Context) error {
		return c.String(http.StatusOK, "test")
	})

	// Valid credentials
	auth = basic + " " + base64.StdEncoding.EncodeToString([]byte("joe:secret"))
	req.Header.Set(echo.HeaderAuthorization, auth)
	assert.NoError(t, h(c))

	// Case-insensitive header scheme
	auth = strings.ToUpper(basic) + " " + base64.StdEncoding.EncodeToString([]byte("joe:secret"))
	req.Header.Set(echo.HeaderAuthorization, auth)
	assert.NoError(t, h(c))

	// Invalid credentials
	auth = basic + " " + base64.StdEncoding.EncodeToString([]byte("joe:invalid-password"))
	req.Header.Set(echo.HeaderAuthorization, auth)
	he := h(c).(*echo.HTTPError)
Repository: labstack/echo
File: middleware/util.go (Go)

Code before:
		domComp[i], domComp[opp] = domComp[opp], domComp[i]
	}
	for i := len(patComp)/2 - 1; i >= 0; i-- {
		opp := len(patComp) - 1 - i
		patComp[i], patComp[opp] = patComp[opp], patComp[i]
	}

	for i, v := range domComp {
		if len(patComp) <= i {
			return false
		}
		p := patComp[i]
		if p == "*" {
			return true
		}
		if p != v {
			return false
		}
	}
	return false
}

func randomString(length uint8) string {
	charset := "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"

	bytes := make([]byte, length)
	_, err := rand.Read(bytes)
	if err != nil {
		// we are out of random. let the request fail
		panic(fmt.Errorf("echo randomString failed to read random bytes: %w", err))
	}
	for i, b := range bytes {
		bytes[i] = charset[b%byte(len(charset))]
	}
	return string(bytes)
}


Review comment (suggestion):
`bufio.Reader` is not thread-safe and `sync.Pool` perf better than mutex.

Code after:
	for i := len(domComp)/2 - 1; i >= 0; i-- {
		opp := len(domComp) - 1 - i
		domComp[i], domComp[opp] = domComp[opp], domComp[i]
	}
	for i := len(patComp)/2 - 1; i >= 0; i-- {
		opp := len(patComp) - 1 - i
		patComp[i], patComp[opp] = patComp[opp], patComp[i]
	}

	for i, v := range domComp {
		if len(patComp) <= i {
			return false
		}
		p := patComp[i]
		if p == "*" {
			return true
		}
		if p != v {
			return false
		}
	}
	return false
}

// https://tip.golang.org/doc/go1.19#:~:text=Read%20no%20longer%20buffers%20random%20data%20obtained%20from%20the%20operating%20system%20between%20calls
var randomReaderPool = sync.Pool{New: func() interface{} {
	return bufio.NewReader(rand.Reader)
}}

const randomStringCharset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
const randomStringCharsetLen = 52 // len(randomStringCharset)
const randomStringMaxByte = 255 - (256 % randomStringCharsetLen)

func randomString(length uint8) string {
	reader := randomReaderPool.Get().(*bufio.Reader)
	defer randomReaderPool.Put(reader)

	b := make([]byte, length)
	r := make([]byte, length+(length/4)) // perf: avoid read from rand.Reader many times
	var i uint8 = 0

	for {
		_, err := io.ReadFull(reader, r)
		if err != nil {
			panic("unexpected error happened when reading from bufio.NewReader(crypto/rand.Reader)")
		}
		for _, rb := range r {
			if rb > randomStringMaxByte {
				// Skip this number to avoid bias.
				continue
			}
Repository: labstack/echo
File: middleware/proxy.go (Go)

Code before:

			retries := config.RetryCount
			for {
				var tgt *ProxyTarget
				var err error
				if isTargetProvider {
					tgt, err = provider.NextTarget(c)
					if err != nil {
						return config.ErrorHandler(c, err)
					}
				} else {
					tgt = config.Balancer.Next(c)
				}

				c.Set(config.ContextKey, tgt)

				//If retrying a failed request, clear any previous errors from
				//context here so that balancers have the option to check for
				//errors that occurred using previous target
				if retries < config.RetryCount {
					c.Set("_error", nil)
				}

				// Reuse context from echo request
				// This is needed to access context values in modifyResponse.
				req = req.WithContext(c.Request().Context())

				// Proxy
				switch {
				case c.IsWebSocket():
					proxyRaw(tgt, c).ServeHTTP(res, req)
				case req.Header.Get(echo.HeaderAccept) == "text/event-stream":
				default:
					proxyHTTP(tgt, c, config).ServeHTTP(res, req)
				}

				err, hasError := c.Get("_error").(error)
				if !hasError {
					return nil
				}

				retry := retries > 0 && config.RetryFilter(c, err)
				if !retry {
					return config.ErrorHandler(c, err)
				}

				retries--
			}
		}
	}
}

Review comment (bug):
I do not understand what this line does. `req` variable is created at line 321 `req := c.Request()`  I do not see any point there that request is replaced with somethings else or there is that method where that `req` context is being changed?

Please provide testcase or small example that I can try with debugger

Code after:

			retries := config.RetryCount
			for {
				var tgt *ProxyTarget
				var err error
				if isTargetProvider {
					tgt, err = provider.NextTarget(c)
					if err != nil {
						return config.ErrorHandler(c, err)
					}
				} else {
					tgt = config.Balancer.Next(c)
				}

				c.Set(config.ContextKey, tgt)

				//If retrying a failed request, clear any previous errors from
				//context here so that balancers have the option to check for
				//errors that occurred using previous target
				if retries < config.RetryCount {
					c.Set("_error", nil)
				}

				// This is needed for ProxyConfig.ModifyResponse and/or ProxyConfig.Transport to be able to process the Request
				// that Balancer may have replaced with c.SetRequest.
				req = c.Request()

				// Proxy
				switch {
				case c.IsWebSocket():
					proxyRaw(tgt, c).ServeHTTP(res, req)
				case req.Header.Get(echo.HeaderAccept) == "text/event-stream":
				default:
					proxyHTTP(tgt, c, config).ServeHTTP(res, req)
				}

				err, hasError := c.Get("_error").(error)
				if !hasError {
					return nil
				}

				retry := retries > 0 && config.RetryFilter(c, err)
				if !retry {
					return config.ErrorHandler(c, err)
				}

				retries--
			}
		}
	}
}
Repository: labstack/echo
File: middleware/basic_auth.go (Go)

Code before:
			l := len(basic)

			if len(auth) > l+1 && strings.EqualFold(auth[:l], basic) {
				// Invalid base64 shouldn't be treated as error
				// instead should be treated as invalid client input
				b, err := base64.StdEncoding.DecodeString(auth[l+1:])
				if err != nil {
					return echo.NewHTTPError(http.StatusBadRequest).SetInternal(err)
				}

				cred := string(b)
				user, pass, ok := strings.Cut(cred, ":")
				if ok {
					// Verify credentials
					valid, err := config.Validator(user, pass, c)
					if err != nil {
						return err
					} else if valid {
						return next(c)
					}
				}
			}

			// Need to return `401` for browsers to pop-up login box.
			// Realm is case-insensitive, so we can use "basic" directly. See RFC 7617.
			c.Response().Header().Set(echo.HeaderWWWAuthenticate, basic+" realm="+strconv.Quote(config.Realm))
			return echo.ErrUnauthorized
		}
	}
}


Review comment (suggestion):
`strconv.Quote` could be moved to line 68 so it would be done once when middleware is created.

Code after:
			}

			auth := c.Request().Header.Get(echo.HeaderAuthorization)
			l := len(basic)

			if len(auth) > l+1 && strings.EqualFold(auth[:l], basic) {
				// Invalid base64 shouldn't be treated as error
				// instead should be treated as invalid client input
				b, err := base64.StdEncoding.DecodeString(auth[l+1:])
				if err != nil {
					return echo.NewHTTPError(http.StatusBadRequest).SetInternal(err)
				}

				cred := string(b)
				user, pass, ok := strings.Cut(cred, ":")
				if ok {
					// Verify credentials
					valid, err := config.Validator(user, pass, c)
					if err != nil {
						return err
					} else if valid {
						return next(c)
					}
				}
			}

			// Need to return `401` for browsers to pop-up login box.
			// Realm is case-insensitive, so we can use "basic" directly. See RFC 7617.
			c.Response().Header().Set(echo.HeaderWWWAuthenticate, basic+" realm="+quotedRealm)
			return echo.ErrUnauthorized
		}
	}
}
Repository: langgenius/dify
File: api/services/conversation_variable_updater.py (Python)

Code before:
from sqlalchemy import select
from sqlalchemy.orm import Session

from core.variables.variables import Variable
from extensions.ext_database import db
from models import ConversationVariable


class ConversationVariableUpdaterImpl:
    def update(self, conversation_id: str, variable: Variable):
        stmt = select(ConversationVariable).where(
            ConversationVariable.id == variable.id, ConversationVariable.conversation_id == conversation_id
        )
        with Session(db.engine) as session:
            row = session.scalar(stmt)
            if not row:
                raise ValueError("conversation variable not found in the database")
            row.data = variable.model_dump_json()
            session.commit()

    def flush(self):
        pass


def conversation_variable_updater_factory() -> ConversationVariableUpdaterImpl:
    return ConversationVariableUpdaterImpl()


Review comment (suggestion):
I prefer not using `ValueError` for business logic invariance violation. The `ValueError` exception is also used by the Python interpreter, so catching it results in other program errors got caught, too.

If the change is to provide better error hint while the invariance is violated, I suggest defining a custom exception class inheriting from `BaseHTTPException`.

Code after:
from sqlalchemy import select
from sqlalchemy.orm import Session

from core.variables.variables import Variable
from extensions.ext_database import db
from models import ConversationVariable


class ConversationVariableNotFoundError(Exception):
    pass


class ConversationVariableUpdaterImpl:
    def update(self, conversation_id: str, variable: Variable) -> None:
        stmt = select(ConversationVariable).where(
            ConversationVariable.id == variable.id, ConversationVariable.conversation_id == conversation_id
        )
        with Session(db.engine) as session:
            row = session.scalar(stmt)
            if not row:
                raise ConversationVariableNotFoundError("conversation variable not found in the database")
            row.data = variable.model_dump_json()
            session.commit()

    def flush(self) -> None:
        pass


def conversation_variable_updater_factory() -> ConversationVariableUpdaterImpl:
    return ConversationVariableUpdaterImpl()
Repository: langgenius/dify
File: web/tsslint.config.ts (TypeScript)

Code before:
import { defineConfig, importESLintRules } from '@tsslint/config'

// npx tsslint-docgen

export default defineConfig({
  rules: {
    rules: {
      ...await importESLintRules({
        'react-x/no-leaked-conditional-rendering': 'warn',
      }),
    },
  },
})


Review comment (suggestion):
The rules layer is redundant. (Even if it has been supported in tsslint).

```suggestion
    ...await importESLintRules({
      'react-x/no-leaked-conditional-rendering': 'warn',
    }),
```

Code after:
import { defineConfig, importESLintRules } from '@tsslint/config'

// Run `npx tsslint-docgen` to generate documentation for the configured rules.

export default defineConfig({
  rules: {
    ...await importESLintRules({
      'react-x/no-leaked-conditional-rendering': 'warn',
    }),
  },
})
Repository: lapce/lapce
File: lapce-data/src/movement.rs (Rust)

Code before:
                format_index(index as i64 - count as i64, len, recursive)
            }
            Movement::Down => {
                format_index(index as i64 + count as i64, len, recursive)
            }
            Movement::Line(position) => match position {
                LinePosition::Line(n) => format_index(*n as i64, len, recursive),
                LinePosition::First => 0,
                LinePosition::Last => len - 1,
            },
            _ => index,
        }
    }
}

pub fn remove_n_at<T>(v: &mut Vec<T>, index: usize, n: usize) {
    match n.cmp(&1) {
        Ordering::Equal => {
            v.remove(index);
        }
        Ordering::Greater => {
            v.drain(index..index + n);
        }
        _ => (),
    };
}

fn format_index(index: i64, len: usize, recursive: bool) -> usize {
    if recursive {
        if index >= len as i64 {
            (index % len as i64) as usize
        } else if index < 0 {
            len - (-index % len as i64) as usize
        } else {
            index as usize
        }
    } else if index >= len as i64 {
        len - 1
    } else if index < 0 {
        0
    } else {
        index as usize
    }
}


Review comment (suggestion):
While this change is unrelated, rustfmt literally wouldn't let me save the file without making it. I could have chosen not to comit it, but I prefer well-formatted code.
And while I was at it, I replaced `splice` with `drain`, which it uses internally, so this code should be equivalent, but strictly simpler.

Code after:
            // Select the next entry/line
            Movement::Down if wrapping => (index + count) % len,
            Movement::Down => (index + count).min(last),

            // Selects the previous entry/line
            Movement::Up if wrapping => (index + (len.saturating_sub(count))) % len,
            Movement::Up => (index.saturating_sub(count)),

            Movement::Line(position) => match position {
                // Selects the nth line
                LinePosition::Line(n) => (*n).min(last),
                LinePosition::First => 0,
                LinePosition::Last => last,
            },
            _ => index,
        }
    }
}

pub fn remove_n_at<T>(v: &mut Vec<T>, index: usize, n: usize) {
    match n.cmp(&1) {
        Ordering::Equal => {
            v.remove(index);
        }
        Ordering::Greater => {
            v.drain(index..index + n);
        }
        _ => (),
    };
}

#[cfg(test)]
mod test {
    use crate::movement::Movement;

    #[test]
    fn test_wrapping() {
        // Move by 1 position
        // List length of 1
        assert_eq!(0, Movement::Up.update_index(0, 1, 1, true));
        assert_eq!(0, Movement::Down.update_index(0, 1, 1, true));

        // List length of 5
        assert_eq!(4, Movement::Up.update_index(0, 5, 1, true));
        assert_eq!(1, Movement::Down.update_index(0, 5, 1, true));

        // Move by 2 positions
        // List length of 1
        assert_eq!(0, Movement::Up.update_index(0, 1, 2, true));
        assert_eq!(0, Movement::Down.update_index(0, 1, 2, true));
Repository: lapce/lapce
File: lapce-data/src/editor/commands/indent_line.rs (Rust)

Code before:
use xi_rope::RopeDelta;

use crate::{
    buffer::data::{BufferDataListener, EditableBufferData},
    movement::{Cursor, Selection},
};
use super::indentation;

pub struct IndentLineCommand<'a> {
    pub(super) selection: Option<Selection>,
    pub(super) cursor: &'a mut Cursor,
    pub(super) tab_width: usize,
}

impl<'a> IndentLineCommand<'a> {
    pub fn execute<L: BufferDataListener>(
        self,
        buffer: EditableBufferData<'a, L>,
    ) -> Option<RopeDelta> {
        let Self {
            selection,
            cursor,
            tab_width,
        } = self;

        Some(indentation::create_multi_edits(
            buffer, selection, cursor, tab_width, edit_one_line
        ))
    }
}

fn edit_one_line<'s, 'b, L: BufferDataListener>(
    buffer: &EditableBufferData<'b, L>,
    offset: usize,
    indent: &'s str,
    tab_width: usize,
) -> Option<(Selection, &'s str)> {
    Some(indentation::create_edit(buffer, offset, indent, tab_width))
}

#[cfg(test)]
mod test {
    use crate::editor::commands::{test::MockEditor, EditCommandKind};

    #[test]
    fn indent_single_line() {
        let mut editor = MockEditor::new("line\n<$0>foo</$0>\nthird line");

        editor.command(EditCommandKind::IndentLine { selection: None });

        assert_eq!("line\n    <$0>foo</$0>\nthird line", editor.state());

Review comment (suggestion):
I like the strategy here, but this makes reading the code a lot more difficult. For one, the implementation is now syntactically identical to outdenting, which is just confusing. I think this could be remedied by renaming `edit_one_line` to `indent/outdent_one_line`

Code after:
use xi_rope::RopeDelta;

use crate::{
    buffer::data::{BufferDataListener, EditableBufferData},
    movement::{Cursor, Selection},
};
use super::indentation;

pub struct IndentLineCommand<'a> {
    pub(super) selection: Option<Selection>,
    pub(super) cursor: &'a mut Cursor,
    pub(super) tab_width: usize,
}

impl<'a> IndentLineCommand<'a> {
    pub fn execute<L: BufferDataListener>(
        self,
        buffer: EditableBufferData<'a, L>,
    ) -> Option<RopeDelta> {
        let Self {
            selection,
            cursor,
            tab_width,
        } = self;

        Some(indentation::execute(
            buffer, selection, cursor, tab_width, indent_one_line
        ))
    }
}

fn indent_one_line<'s, 'b, L: BufferDataListener>(
    buffer: &EditableBufferData<'b, L>,
    offset: usize,
    indent: &'s str,
    tab_width: usize,
) -> Option<(Selection, &'s str)> {
    Some(indentation::create_edit(buffer, offset, indent, tab_width))
}

#[cfg(test)]
mod test {
    use crate::editor::commands::{test::MockEditor, EditCommandKind};

    #[test]
    fn indent_single_line() {
        let mut editor = MockEditor::new("line\n<$0>foo</$0>\nthird line");

        editor.command(EditCommandKind::IndentLine { selection: None });

        assert_eq!("line\n    <$0>foo</$0>\nthird line", editor.state());
Repository: lapce/lapce
File: lapce-ui/src/logging.rs (Rust)

Code before:
        let mut iter = module.split('=');
        if let Some(val) = iter.next() {
            if let Some(level) = iter.next() {
                println!("module='{val}', level='{level}'");
                // "module=level"
                //
                // NOTE: The dash characters in crate names are converted into
                // underscores by the compiler.  For example, path to this
                // module will be "lapce_ui::loggings".
                dispatch = set_level(dispatch, Some(val), level);
            } else {
                println!("level='{val}' for all modules");
                // just "level"
                dispatch = set_level(dispatch, None, val);
            }
        }
    }
    dispatch
}

pub(super) fn override_log_levels(dispatch: Dispatch) -> Dispatch {
    match std::env::var("RUST_LOG") {
        // Not an error if the env var does not exist.
        Err(std::env::VarError::NotPresent) => dispatch,
        Err(std::env::VarError::NotUnicode(val)) => {
            // TODO: throw an error? logger is not configured yet
            let val = val.to_string_lossy();
            eprintln!("RUST_LOG: ignored invalid unicode value: '{val}'");
            dispatch
        },
        Ok(val) => parse_log_levels(&val, dispatch),
    }
}


Review comment (performance):
I think a "noisy" ignore is fine here. Logging isn't entirely critical, and printing to console should be enough to notify someone.

If we would want to keep this error in the logs, we could change this function to initialize the logger, and then log this error, but I don't think we need the extra complexity.

Code after:
            println!("module='{module}', level='{level}'");
            // "module=level"
            //
            // NOTE: The dash characters in crate names are converted into
            // underscores by the compiler.  For example, path to this
            // module will be "lapce_ui::loggings".
            dispatch = set_level(dispatch, Some(module), level);
        } else {
            println!("level='{section}' for all modules");
            // just "level"
            dispatch = set_level(dispatch, None, section);
        }
    }
    dispatch
}

pub(super) fn override_log_levels(dispatch: Dispatch) -> Dispatch {
    match std::env::var("RUST_LOG") {
        // Not an error if the env var does not exist.
        Err(std::env::VarError::NotPresent) => dispatch,
        Err(std::env::VarError::NotUnicode(val)) => {
            let val = val.to_string_lossy();
            eprintln!("RUST_LOG: ignored invalid unicode value: '{val}'");
            dispatch
        },
        Ok(val) => parse_log_levels(&val, dispatch),
    }
}